Last modified: 2014-02-10 16:41:31 UTC

Wikimedia Bugzilla is closed!

Wikimedia has migrated from Bugzilla to Phabricator. Bug reports should be created and updated in Wikimedia Phabricator instead. Please create an account in Phabricator and add your Bugzilla email address to it.
Wikimedia Bugzilla is read-only. If you try to edit or create any bug report in Bugzilla you will be shown an intentional error message.
In order to access the Phabricator task corresponding to a Bugzilla report, just remove "static-" from its URL.
You could still run searches in Bugzilla or access your list of votes but bug reports will obviously not be up-to-date in Bugzilla.
Bug 56418 - Invalid HTML in Echo notification list pop-up
Invalid HTML in Echo notification list pop-up
Status: REOPENED
Product: MediaWiki extensions
Classification: Unclassified
Echo (Other open bugs)
unspecified
All All
: Unprioritized normal with 1 vote (vote)
: ---
Assigned To: Nobody - You can work on this!
:
: 57432 61121 (view as bug list)
Depends on:
Blocks: code_quality
  Show dependency treegraph
 
Reported: 2013-10-31 13:03 UTC by Danny B.
Modified: 2014-02-10 16:41 UTC (History)
9 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments
Shrink the clickable regions (30.30 KB, image/png)
2013-11-25 11:31 UTC, TMg
Details
Other possible solution for clickable regions (11.77 KB, image/png)
2013-11-26 22:06 UTC, TMg
Details

Description Danny B. 2013-10-31 13:03:04 UTC
Invalid HTML in Echo notification list pop-up.

http://dev.w3.org/html5/markup/a.html#a-constraints says:
"The interactive element a must not appear as a descendant of the a element."

The current code is:

(trimmed)

<ul class="mw-echo-notifications">
  <li class="mw-echo-notification">
    <a class="mw-echo-notification-wrapper"> <!-- wrapping [a] -->
      <div class="mw-echo-state">
        <img class="mw-echo-icon">
        <div class="mw-echo-content">
          <div class="mw-echo-title">
            <a class="mw-echo-grey-link">...</a> ... <!-- nested [a] -->
          </div>
          <div class="mw-echo-notification-footer">
            před 2 hodinami | 
            <a class="mw-echo-notification-secondary-link mw-echo-grey-link" >...</a> <!-- nested [a] -->
          </div>
          <a class="mw-echo-notification-primary-link">...</a> <!-- nested [a] -->
        </div>
      </div>
    </a>
  </li>
</ul>
Comment 1 Bartosz Dziewoński 2013-10-31 13:32:30 UTC
This was done by me in https://gerrit.wikimedia.org/r/#/c/77824/ as the solution to bug 52319, detailed rationale is included in the commit message.

tl;dr:
a) This is not HTML markup, but dynamically generated HTML DOM;
b) All browsers I know of support this, including IE6;
c) It is also fully valid XHTML, which likely explains why this works.

I suggest WONTFIXing this bug, but I am ready to be convinced if you come up with irrefutable arguments.
Comment 2 Bartosz Dziewoński 2013-10-31 13:34:25 UTC
I removed two of the tracking bugs:
* bug 209 is about markup only, and this is not markup
* bug 19719 is about HTML5, and this is not related to any of the '5' features

Bug 700 being about code quality issues is a valid tracker assuming we consider this to be an issue (which I don't).
Comment 3 spage 2013-10-31 17:02:22 UTC
The WMF core features team tracks this bug on Mingle card https://mingle.corp.wikimedia.org/projects/flow/cards/394, but people from the community are welcome to contribute here and in Gerrit.
Comment 4 Danny B. 2013-11-01 13:59:33 UTC
(In reply to comment #1)
> c) It is also fully valid XHTML, which likely explains why this works.

How it can be valid XHTML, when <a>'s can't be nested in XHTML as well?

http://www.w3.org/TR/xhtml1/dtds.html#dtdentry_xhtml1-transitional.dtd_a
Comment 5 Bartosz Dziewoński 2013-11-01 16:10:51 UTC
I haven't been able to find a document that would call nesting <a> elements invalid in XHTML, so I presumed it to be valid. And as far as I understand it the DTD does not technically disallow a structure like <a><em><a>…</a></em></a> (and this is the kind of structure used here), it merely disallows <a><a>…</a></a>.

Either way, that was just a side point. My main point is that this works.
Comment 6 Bartosz Dziewoński 2013-11-22 18:03:17 UTC
*** Bug 57432 has been marked as a duplicate of this bug. ***
Comment 7 Bartosz Dziewoński 2013-11-22 18:04:11 UTC
Per the report in bug 57432, old Opera 12 has issues when trying to open that link in a new tab; it open correctly when you just click it, however. Not sure if that is really something we should concern ourselves with, but let's reopen.
Comment 8 TMg 2013-11-25 11:31:46 UTC
Created attachment 13896 [details]
Shrink the clickable regions

Your argument is to be pragmatic. I love pragmatism. But this is going to far. Nested links are illegal.[1] If fundamental stuff like this is ignored, what's the point in having a spec?

It may be true that it works in current versions of all major browsers. But I still remember a time where nested links were completely ignored. Are you sure these browser versions aren't used any more? It also causes a number of other problems. For example, simple CSS selectors like
.mw-echo-notifications a { background:red !important; }
cause strange and probably unexpected results. Tooltips become misleading:
<a href="a" title="Click here to go to a"><a href="b">

Please see my longer comment about the clickable areas in the overlay.[2] My favorite solution is to shrink the clickable regions as shown in the attached screenshot. This will solve this bug as well as other issues.

[1]http://www.w3.org/TR/html401/struct/links.html#h-12.2.2
[2]http://en.wikipedia.org/wiki/Wikipedia_talk:Notifications/Archive_4#When_are_we_getting_diffs.2C_Part_Deux
Comment 9 Bartosz Dziewoński 2013-11-25 18:29:58 UTC
I'm not sure about that, I think it'd need some sort of indicator which parts are clickable.

We could come up with some cleverer solution that wouldn't need the stupid nesting, maybe. The original one (before I implemented this) was to handle the "outer" link with JavaScript event handlers, but that means that middle-click etc. won't work without some careful separate handling and probably won't be consistent with browsers' behavior anyway. (See bug 52319 for discussion about that.)

But anyway, I won't have time to work on this anytime soon, sorry (note that I'm just a volunteer). We could consider reverting my patch, of course, but I believe that would be a step back (since the nesting is not causing any major issues – really, somebody would have complained if the links didn't work! – the only thing I've heard is that middle-click doesn't work well on Opera, and I only heard it from you here and this really is not a major issue :) ).
Comment 10 TMg 2013-11-26 22:06:43 UTC
Created attachment 13915 [details]
Other possible solution for clickable regions

(In reply to comment #9)
> need some sort of indicator which parts are clickable.

In my first screenshot the indicator is the text. Regular text = clickable, smaller text = not clickable. I'm aware that it's not a perfect solution. There are others, see my second screenshot.

On non-touch devices the mouse pointer is a very strong indicator. That's the other issue I spoke about: Currently the mouse pointer never changes. Everything is clickable. There is no separation between the individual notifications and no separation between the diff link and the rest of a notification. This is an other issue but can be fixed along with this one.

> The original [...] was to handle the "outer" link with JavaScript event
> handlers, but that means that middle-click etc. won't work

I know. Don't get me wrong. The current solution is much better compared to the JavaScript one. It should not be reverted. However, there must be a solution that does not violates specs.

> not a major issue

As I said: I understand, but the same time I consider invalid HTML a major issue.
Comment 11 Bartosz Dziewoński 2014-02-10 16:41:31 UTC
*** Bug 61121 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links