Page MenuHomePhabricator

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

Description

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>


Version: unspecified
Severity: normal

Details

Reference
bz56418

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

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.

[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

Attached:

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.

Attached:

  • 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