Last modified: 2014-02-10 16:41:31 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>
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.
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).
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.
(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
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.
*** Bug 57432 has been marked as a duplicate of this bug. ***
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.
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
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 :) ).
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.
*** Bug 61121 has been marked as a duplicate of this bug. ***
