331959 - Make nested links work

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P1)

Description

[Jonathan Watt [:jwatt]]

Splitting this off from bug 211916.

When we're dealing with HTML we don't generate nested links from markup because the HTML parser closes any currently open anchor element when it encounters another opening <a> tag. Scripts can create nested links, but the linking behaviour breaks on them (see upcoming testcase). When dealing with XLink, we can more easily get nested links, and we should handle them sensibly. I think the correct way to handle this is to set nsEventStatus_eConsumeNoDefault on events after we handle them in PostHandleEventForAnchors.

I want to do this as a separate bug from bug 211916 so we can work out any bugs it may introduce before merging the link handling code etc.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: testcase - nested links

The top half is relevant to this bug.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

This is a dup, should just find the right bug.

Comment 3

[Jonathan Watt [:jwatt]]

There's bug 110817, but that's for XLink, and I'm specifically dealing with HTML here (before going on to merge the XLink and HTML handling).

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: patch PostHandleEventForAnchors

This seems to work, and I don't see any ill effects (yet).

I've nuked IsArea because the check is no longer necessary since the event get's NoDefault set before it's passed up from the target <area> to any parent links. The lower half of the testcase tests that it works correctly.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 216519 [details] [diff] [review]
patch PostHandleEventForAnchors


>       case NS_FOCUS_CONTENT:
>       {
>         nsAutoString target;
>         GetAttr(kNameSpaceID_None, nsHTMLAtoms::target, target);
>         if (target.IsEmpty()) {
>           GetBaseTarget(target);
>         }
>         rv = TriggerLink(aVisitor.mPresContext, eLinkVerb_Replace,
>                          hrefURI, target, PR_FALSE, PR_TRUE);
>+
>+        // XXXjwatt: why not set eConsumeNoDefault - how is it that parent
>+        // nodes don't snatch focus?

Focus doesn't bubble, right?

Comment 6

[[e-mail]]

Comment on attachment 216519 [details] [diff] [review]
patch PostHandleEventForAnchors

I dug out the patch I made ages ago to try to fix this for comparison:

>-IsArea(nsIContent *aContent)
Yes :-)

>             // single-click
>             nsUIEvent actEvent(NS_IS_TRUSTED_EVENT(aVisitor.mEvent),
>                                NS_UI_ACTIVATE, 1);
>             nsEventStatus status = nsEventStatus_eIgnore;
> 
>             rv = shell->HandleDOMEventWithTarget(this, &actEvent, &status);
>-            aVisitor.mEventStatus = status;
Note that as you have fixed the status for an NS_UI_ACTIVATE event I don't see why you should need to fix it again here.

>       // Set the status bar the same for focus and mouseover
>       case NS_MOUSE_ENTER_SYNTH:
>+        // XXXjwatt: why do we do this? we need more comments!
>         aVisitor.mEventStatus = nsEventStatus_eConsumeNoDefault;
>       case NS_FOCUS_CONTENT:
I guess because mouseover bubbles and focus doesn't?

>       case NS_MOUSE_EXIT_SYNTH:
>       {
>         aVisitor.mEventStatus = nsEventStatus_eConsumeNoDefault;
>         rv = LeaveLink(aVisitor.mPresContext);
>       }
IMHO we need something similar for NS_BLUR_CONTENT too (but again, that doesn't bubble).

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: patch PostHandleEventForAnchors

smaug/neil: uh, yeah, not bubbling is probably key. :-)

> Note that as you have fixed the status for an NS_UI_ACTIVATE event I don't see
> why you should need to fix it again here.

(Note I've changed that line in this patch.) Anyway, the default action of a click on an anchor is to dispatch a DOMActivate event. Once that's done, the default action for the click event has been carried out. It seems to me that once a default action has been carried out for a given event, we should be preventing any further default actions occuring for that event unless we have a good (and well commented) reason not to. I see no reason to perform any more default actions on click events an anchor has handled, so bring on the NoDefault! Whether the DOMActivate action succeeded or not, it was sent, and that's the default. 

I added in a case for NS_BLUR_CONTENT BTW.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Does this patch enable fixing of bug 325652, or any of the bugs it depends on?

Comment 9

[Boris Zbarsky [:bzbarsky]]

This is basically a duplicate of bug 127903...,

Comment 10

[Jonathan Watt [:jwatt]]

Attachment: patch with changes to browser.js (for bug 325652)

Gavin: this bug *requires* bug 325652 to be fixed by its very nature (it fixes the broken behaviour that required the parity hack). Here's a new patch with the changes to browser.js in case you wanted to see them.

Boris: I don't see why. The way I see this bug being fixed is to set nsEventStatus_eConsumeNoDefault on events when they're handled in nsGenericHTMLElement::PostHandleEventForAnchors so that the same method doesn't act on events a second time for links further up the parent chain. The way I imagine bug 127903 being fixed is to do the same in PostHandleEvent for the submit button (I haven't actually looked at the code, so that could be off) to prevent PostHandleEventForAnchors handling the event.

Note this patch also fixes bug 110817 and bug 266958. Still thinking through the code paths and testing before requesting review, but so far I haven't had any problems.

Comment 11

[Jonathan Watt [:jwatt]]

Oh, and it *doesn't* fix bug 127903 BTW.

Comment 12

[Jonathan Watt [:jwatt]]

Actually the nested Xlinks bug (bug 110817) has been partially fixed by something else. (I don't know what or when.) It's completely fixed in SeaMonkey trunk, and the only reason it's not fixed in Firefox trunk is because of the hack that bug 325652 exists to back out. Hence the reason my patch "fixes" the nested XLink bug in Firefox is actually because it includes the backout of the hack. Clearly I'm not touching any other XLink code.

Comment 13

[[e-mail]]

XLinks are probably handled in nsXMLElement::PostHandleEvent()

Comment 14

[Jonathan Watt [:jwatt]]

Yeah, it is. I'm currently working on merging the XLink link and HTML link code. I see now why things are working in XLink land. It's because the leaf-most link sets nsEventStatus_eConsumeDoDefault on the click event in PostHandleEvent, but PostHandleEvent only handles events with nsEventStatus_eIgnore. Thus any links further up the parent chain won't handle the event in PostHandleEvent.

In HTML land on the other hand, PostHandleEventForAnchors dispatches a DOMActivate event with nsEventStatus_eIgnore. Nothing, including PostHandleEventForAnchors when it handles it next time it sees it, ever sets nsEventStatus_eConsumeDoDefault on this event. Therefore the activate event goes up the parent nested links and TriggerLink is called for each link - the rootmost link wins.

Comment 15

[Jonathan Watt [:jwatt]]

Attachment: patch with NoDefault added for mousedown

I noticed that the root-most link in a bunch of nested links gets the CSS "active:" styling when you mousedown on one of its decendent links. Now setting NoDefault on mousedown events too to stop that from happening. It actually doesn't work, but I think it's what we should do anyway, baring a good reason not to. Not sure where the active styling is coming from.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Jonathan, I was using bug 127903 to track changes to all link-like (<a>, <area>, <button>, <input type="button">) things to make sure they all work together correctly; the same changes need to be made to all three, and should be made together in my opinion.  A number of the dups of that bug are exact duplicates of this one, if you look at them.

That is, I don't see why we're just changing links, but if we insist on doing that I guess that's ok as long as we fix bug 127903 soon after.

Note that I _do_ think that the JS changes should be in bug 325652, not here.

As for :active, it's hierarchical -- all ancestors of an :active node are also :active.  That has nothing to do with what links are or are not doing.  So I'm not sure what the point of the mousedown change is, but the comment there is definitely wrong.

Comment 17

[Boris Zbarsky [:bzbarsky]]

That said, we _do_ want to call SetContentState with ACTIVE only on the furthest-from-root anchor.  So doing preventDefault there is probably correct; the comment just needs tweaking.

Comment 18

[Jonathan Watt [:jwatt]]

I'm really not keen on taking on the work for <button> and <input type="button/submit"> (<area> is fixed by this). As always, trying to fix one bug has led me into successively deeper layers of bugs that need to/should be fixed first. I'm currently down at about the fourth or fifth layer and have no desire to get any deeper. Especially as I'm sure changing <button> and <input> to preventDefault() will involve even more involved testing/thinking/code path than this change to <a>.

> Note that I _do_ think that the JS changes should be in bug 325652, not here.

Suits me. I was going to send that part over but thought it would be useful here too for anyone here who wants to test.

> That said, we _do_ want to call SetContentState with ACTIVE only on the
> furthest-from-root anchor.  So doing preventDefault there is probably correct;
> the comment just needs tweaking.

Sure. Thanks, I didn't realize :active was hierarchical. Saves me chasing my tail.

Thanks for the comments!

Comment 19

[Boris Zbarsky [:bzbarsky]]

Is fixing this really necessary for the XLink stuff?  I'd really like us to fix and test this together with buttons instead of doing it piecemeal.

Comment 20

[Jonathan Watt [:jwatt]]

Not absolutely. Since I'm moving nsXMLElement::PostHandleEvent up to nsGenericElement, sicking asked if I could merge it with nsGenericHTMLElement::PostHandleEventForAnchors. The two are all over the place with their nsEventStatus story, and that needs sorted out before a merge.

Okay, I'm done for the day, but I'll take a look through the <form>, <button> code etc. tomorrow. If nothing too funky is going on I'll consider taking on bug 127903 as well. I'd still rather make this a separate check-in though.

Comment 21

[Jonathan Watt [:jwatt]]

Attachment: current diff

Since the issue of whether default actions should themselves be allowed to call preventDefault() is currently being contested, I'm going to leave this alone for now. For my (or other's) future reference, this is the latest state of the patch I was working on.

Comment 22

[Jonathan Watt [:jwatt]]

Barring the debate, it would probably be ready for review.

Comment 23

[Markus S.]

Bug seems to be fixed in current trunk builds. Tested with Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20061210 SeaMonkey/1.5a

Comment 24

[Ryan VanderMeulen [:RyanVM]]

No, it isn't. If you hover over the jwatt link, the status bar shows the proper location. However, clicking it still goes to Google.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061210 Minefield/3.0a1

Comment 25

[Jonathan Watt [:jwatt]]

I can't see myself coming back to this in the near future, so assigning to [e-mail] for now.

Note http://lists.w3.org/Archives/Public/www-svg/2007Feb/0066.html

Comment 27

[Boris Zbarsky [:bzbarsky]]

Per bug 599339, this may need to block Gecko 2 (due to us now allowing nested links via the HTML parser and this breaking modem configuration screens).  Olli, is this something that we can safely fix for 2.0 at this point?

Comment 28

[Radek 'sysKin' Czyz]

(In reply to comment #0)
> When we're dealing with HTML we don't generate nested links from markup 

For what it's worth, this os no longer the case with html parser. See attachment 478255 [details].

Can I please suggest that this bug became more important now than it was before. It breaks, for example, configuration page of Motorola SB5101 cable modem. It's a popular device all over Australia where Telstra was sending it right and left.

Comment 29

[Boris Zbarsky [:bzbarsky]]

smaug, did we ever decide what behavior we actually want here?

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

I guess the first link, I mean the one somewhere down deep in the DOM
should be activated. And only that one should fire DOMActivate.

Boris, feel free to assign this to me, if you don't have time for this.

Comment 31

[Boris Zbarsky [:bzbarsky]]

I can work on this; I'm not too overburdened with other blockers at the moment.

Comment 32

[Johnny Stenback (:jst)]

I think we need to get to this sooner rather than later. Boris, you still up for fixing this?

And for the record, the reason I think this still blocks is Boris' reference to router config pages no longer working due to intentional changes in how we parse broken HTML.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Yeah, this is still on my plate.  ETA is a few days; it's the top priority on my blocker list right now.

Comment 34

[Chris B.]

Don't feel bad with this bug, Mozilla (which has been open for four years???).  Microsoft has the OPPOSITE problem with Internet Explorer 9 Beta.  In IE 9, the embedded Google / Jwatt links on the top works, but clicking on the black area in the picture does nothing (doesn't link to Google).  The Jwatt link in the picture works with IE 9 Beta.

Comment 35

[Boris Zbarsky [:bzbarsky]]

I filed bug 618414 on a focus issue with buttons-inside-anchors that I'm going to punt on for now.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Oh, and I'm just going to make this work for anchors+inputs; fully rewriting activation to work like html5 (e.g. getting rid of DOMActivate events altogether) is a battle for another day and way earlier in the release cycle.

Comment 37

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

I think this should do the trick.  The basic approach is to treat enter-firing-click as preventing any other default actions and to treat click-firing-activate the same way, then to change anchors to only respond to activate targeted at them, not at some descendant.  The browser fixes are basically fixing bug 325652.  The tests test the core bits; we still need tests for the browser bits.  In particular, we need to test that accel-click on the inner anchor loads it, not the outer, as does the 'open in new tab/window' context menu item.  We also need to test that hovering the inner anchor shows the right thing in the urlbar 'where we're going' preview area.  I've done manual verification of all this; we just need automated tests.  Perhaps we could morph bug 325652 to cover that?

Comment 38

[Boris Zbarsky [:bzbarsky]]

Attachment: Even including test

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 496936 [details] [diff] [review]
Even including test

>diff --git a/content/base/src/nsGenericElement.cpp b/content/base/src/nsGenericElement.cpp
>--- a/content/base/src/nsGenericElement.cpp
>+++ b/content/base/src/nsGenericElement.cpp
>@@ -5247,33 +5247,36 @@ nsGenericElement::PreHandleEventForLinks
> 
>   nsresult rv = NS_OK;
> 
>   // We do the status bar updates in PreHandleEvent so that the status bar gets
>   // updated even if the event is consumed before we have a chance to set it.
>   switch (aVisitor.mEvent->message) {
>   // Set the status bar similarly for mouseover and focus
>   case NS_MOUSE_ENTER_SYNTH:
>-    aVisitor.mEventStatus = nsEventStatus_eConsumeNoDefault;
>-    // FALL THROUGH
>   case NS_FOCUS_CONTENT:
>     if (aVisitor.mEvent->eventStructType != NS_FOCUS_EVENT ||
>         !static_cast<nsFocusEvent*>(aVisitor.mEvent)->isRefocus) {
>       nsAutoString target;
>       GetLinkTarget(target);
>-      nsContentUtils::TriggerLink(this, aVisitor.mPresContext, absURI, target,
>-                                  PR_FALSE, PR_TRUE);
>+      nsContentUtils::TriggerLink(this, aVisitor.mPresContext, absURI,
>+                                  target, PR_FALSE, PR_TRUE);
>+      // Focus events don't bubble, but pre-handling happens all the way
>+      // up the tree.  So we need to set mEventStatus for NS_FOCUS_CONTENT too.
The comment is kind of misleading. What has this to do with bubbling at all?

>+      aVisitor.mEventStatus = nsEventStatus_eConsumeNoDefault;
I'm little bit worried about this change.
un-standard  event.getPreventDefault() would start to return different value
for some focus events.
In DOM 3 Events there will be property call defaultPrevented.

Though, now that I think this, I guess we'll need to limit .defaultPrevented to .preventDefault()
calls happening on content. So .defaultPrevented won't be like .getPreventDefault()
And getPreventDefault() has worked with focus and other simple type events only since 3.6.
So perhaps the change isn't that scary after all.

>   case NS_UI_ACTIVATE:
>     {
>-      nsAutoString target;
>-      GetLinkTarget(target);
>-      nsContentUtils::TriggerLink(this, aVisitor.mPresContext, absURI, target,
>-                                  PR_TRUE, PR_TRUE);
>+      nsCOMPtr<nsIContent> targetContent;
>+      aVisitor.mPresContext->EventStateManager()->
>+        GetEventTargetContent(aVisitor.mEvent, getter_AddRefs(targetContent));
>+      if (targetContent == this) {
>+        nsAutoString target;
>+        GetLinkTarget(target);
>+        nsContentUtils::TriggerLink(this, aVisitor.mPresContext, absURI, target,
>+                                    PR_TRUE, PR_TRUE);
>+      }
Why this change?
Couldn't you just mark aVisitor.mEventStatus = nsEventStatus_eConsumeNoDefault

Comment 40

[Boris Zbarsky [:bzbarsky]]

> What has this to do with bubbling at all?

Nothing; I mostly copy/pasted the comment from the first patch in this bug.  I can remove this comment if you prefer.

> So perhaps the change isn't that scary after all.

OK.  If we had a way to flag the event with some bit that says "don't do link stuff" I could use that here, but I don't think we have a way to do that.

> Couldn't you just mark aVisitor.mEventStatus = nsEventStatus_eConsumeNoDefault

I could; I thought the target check was less invasive in case someone is manually dispatching activate events... but maybe that's a silly worry?

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #40)
> OK.  If we had a way to flag the event with some bit that says "don't do link
> stuff" I could use that here, but I don't think we have a way to do that.
We should have still plenty of bits in nsEvent.flags

> I could; I thought the target check was less invasive in case someone is
> manually dispatching activate events... but maybe that's a silly worry?
Well, we handle only trusted events here anyway, so I wouldn't worry.

Comment 42

[Boris Zbarsky [:bzbarsky]]

> We should have still plenty of bits in nsEvent.flags

Would you prefer I did that instead, at least for the prehandle parts?

Comment 43

[Olli Pettay [:smaug][[e-mail]]]

Would be safer, so yes.

Comment 44

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 496936 [details] [diff] [review]
Even including test

I assume there is a new patch coming.

Comment 45

[Boris Zbarsky [:bzbarsky]]

Attachment: and bug 127903. Make situations in which an anchor or submit control is nested inside another anchor and the inner thing is clicked trigger the inner thing, not both.

Comment 46

[Boris Zbarsky [:bzbarsky]]

smaug, I left the target check in, because otherwise the fake activate events that happen with buttons confuse things.  And I can't just prevent default on those in the button code, because buttons don't actually respond to them!

We can probably simplify this when we remove support for DOMActivate and/or implement HTML5's activation model.

Comment 47

[Boris Zbarsky [:bzbarsky]]

Attachment: Interdiff from previous patch version, in case someone cares

Comment 48

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 499204 [details] [diff] [review]
and bug 127903.  Make situations in which an anchor or submit control is nested inside another anchor and the inner thing is clicked trigger the inner thing, not both.

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

> function hrefAndLinkNodeForClickEvent(event)

>+  let node = event.originalTarget;
>+  while (node && !isHTMLLink(node)) {
>+    node = node.parentNode;
>+  }
>+
>+  if (node)
>+    return [node.href, node];
> 
>   // If there is no linkNode, try simple XLink.
>   let href, baseURI;
>   let node = event.target;

This now re-declares "node". Also I suppose these two should be consistent wrt use of target/originalTarget. r=me with that fixed.

Comment 49

[Boris Zbarsky [:bzbarsky]]

> This now re-declares "node".

Will fix.

> Also I suppose these two should be consistent wrt use of target/originalTarget

I was trying to not change behavior there.  Which way would you like the consistency to go?

Comment 50

[:Gavin Sharp [email: [e-mail]]]

Probably better to just use target, I guess. We don't particularly care about retargeting.

Comment 51

[Boris Zbarsky [:bzbarsky]]

> Probably better to just use target

Done.

Comment 52

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/b7acaa08a9c5 and then pushed followup http://hg.mozilla.org/mozilla-central/rev/b481371cec34 to fix the resulting orange.  There were two orange issues:

1) The new test failed in the harness because the harness uses an iframe too small to allow the whole test to be visible, so the clicks weren't working right.  Fixed the test to deal.

2) The change to prevent default for mousedown broke some of the anchor drag and drop tests.  Fixed by removing that change and using the new event flag on that codepath instead.

Comment 53

[LegNeato]

As per today's meeting, beta 9 will be a time-based release. Marking these all betaN+. Please move it back to beta9+ if  you believe it MUST be in the next beta (ie: trunk is in an unshippable state without this)

Comment 54

[Daniel C]

[e-mail] 	2011-01-04 23:42:57 GMT 	Keywords 	regression 	
CC 	[e-mail], [e-mail], [e-mail], [e-mail], [e-mail], [e-mail], [e-mail], [e-mail], [e-mail], [e-mail], [e-mail] 	
CC 	[e-mail], [e-mail], [e-mail], [e-mail], [e-mail], [e-mail] 	
Blocks 	211916, 325652, 127903 	
Depends on 	622117 	
blocking2.0 	beta9+ 	betaN+ 

What happened there? Browser bug/bugzilla bug/human error?

Comment 55

[Boris Zbarsky [:bzbarsky]]

ccing Christian so he'll actually see comment 54.