Page MenuHomePhabricator

Invalid HTML in Echo notification list pop-up
Closed, ResolvedPublic


Invalid HTML in Echo notification list pop-up. says:
"The interactive element a must not appear as a descendant of the a element."

The current code is:


<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 class="mw-echo-notification-footer">
          před 2 hodinami | 
          <a class="mw-echo-notification-secondary-link mw-echo-grey-link" >...</a> <!-- nested [a] -->
        <a class="mw-echo-notification-primary-link">...</a> <!-- nested [a] -->


Version: unspecified
Severity: normal



Event Timeline

bzimport raised the priority of this task from to Needs Triage.Nov 22 2014, 2:25 AM
bzimport added a project: Notifications.
bzimport set Reference to bz56418.
bzimport added a subscriber: Unknown Object (MLST).
Danny_B created this task.Oct 31 2013, 1:03 PM

This was done by me in as the solution to bug 52319, detailed rationale is included in the commit message.

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, 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?

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.

mr.heat wrote:

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.



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 :) ).

mr.heat wrote:

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. ***
Krinkle added a subscriber: Krinkle.
EBernhardson triaged this task as Lowest priority.Dec 12 2014, 8:29 PM

Fixing this is worthwhile, but any fix must take into consideration T54319: Echo: ctrl+click & middle click and solve that problem as the existing solution does. Unless this causes user visible issues in more places i don't see the collaboration team working on this.

Legoktm moved this task from Backlog to Needs plan on the Notifications board.Jul 6 2015, 9:11 AM
Restricted Application added a project: Collaboration-Team-Triage. · View Herald TranscriptJul 6 2015, 9:11 AM
Devirk moved this task from Needs plan to Needs code on the Notifications board.Jan 11 2016, 12:55 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 11 2016, 12:55 PM
Restricted Application added a project: Growth-Team. · View Herald TranscriptJul 18 2018, 6:44 PM
SBisson moved this task from Inbox to Triaged but Future on the Growth-Team board.Jul 20 2018, 6:12 PM
Catrope closed this task as Resolved.Mar 21 2019, 7:53 PM
Catrope claimed this task.
Catrope added a subscriber: Catrope.

Echo's current HTML DOM no longer contains <a> elements nested inside of <a> elements