596698 - Link target in location bar isn't updated when switching between tabs if link is focused

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect)

Description

[Justin Lebar (not reading bugmail)]

STR:

 Load http://mozilla.org.
 Middle-click on one of the links so it appears in a new tab.
 Move the mouse.
 Press ctrl + shift + t to go back to the previous tab.  Notice that the link you clicked is focused (it has a dotted outline around it).

Expected results:
 
 Link's target isn't shown in location bar.

Actual results:

 Link's target is shown in location bar.

Comment 1

[Drew Willcoxon :adw]

This is the same behavior as when link targets were shown in the status bar: focused links, not only moused over links, are shown.

Is switching tabs really related to the problem?  Or is it just that you think focused links shouldn't be shown in the location bar?

Comment 2

[Justin Lebar (not reading bugmail)]

I experimented with this a bit more, both before and after the change.  These tests are from WinXP:

If you middle-click but have new tabs set to open in the background, then the link stays focused, but the link target is removed from the location bar as soon as you move the mouse off the link.  This is the behavior I'd expect.  (Previously, when link targets were shown in the status bar, the link target would be updated when you hovered over a new link, and then go away when you unhovered that new link.)

If, however, you have new tabs set to open in the background, you middle click on the link, ctrl+tab to the next tab, move the mouse, and then ctrl+tab back to the original tab, the link target remains in the address bar until you mouse in and out of a link, at which point the location bar is cleared.

Our new behavior may or may not match our old behavior, but perhaps it matters more now that the link target is so prominently displayed.

Comment 4

[Drew Willcoxon :adw]

(In reply to comment #2)
> Our new behavior may or may not match our old behavior, but perhaps it matters
> more now that the link target is so prominently displayed.

Fair point.  Alex, Limi, should focused links be shown in the location bar?

Comment 5

[Alex Limi (:limi) — Firefox UX Team]

Yes, I don't think we should show focused links now that it's a bit more prominent, only hovered ones. Unless there are accessibility implications (which I can't see, but who knows :)

Comment 6

[Jesse Ruderman]

If the link is focused by the user (e.g. by pressing Tab using using '), its destination needs to be shown, for accessibility reasons.

But if the link is focused indirectly, for example by switching tabs, it's sensible to not show it.

Comment 7

[Drew Willcoxon :adw]

One relevant layer in the onion is here in nsGenericElement::PreHandleEventForLinks:

http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsGenericElement.cpp#5280

The NS_MOUSE_ENTER_SYNTH case is link hover, and the NS_FOCUS_CONTENT is case is link focus.  Modifying that, I can turn off the focus case.  But I don't know how to tell whether a focus is "indirect."  And since a layer this deep in the onion probably shouldn't be modified to suit Firefox's front-end needs anyway (suck), it doesn't look like the fix for this will be pretty.

Comment 10

[Reece H. Dunn]

This is also noticeable for links that open in new tabs -- that is, they have the form:

    <a target="_blank" href="http://www.mozilla.org">Mozilla</a>

In terms of the cause, it will be because a mouse-enter event will be setting the navigation link, and mouse-leave will be clearing it. However, a mouse-leave event is not being triggered when switching tabs.

This will also cause issues with web pages that use similar events (e.g. changing the links colour on mouse-over).

Playing around a bit I have noticed that a link gets activated when switching *to* a tab:
   1.  open a page in Tab 1 (e.g. www.mozilla.org)
   2.  open a page in Tab 2 to a bugzilla entry with Comments
   3.  hover over a Comment
   4.  switch to Tab 1
   5.  move the mouse to where the [reply] section is on Tab 2
   6.  switch to Tab 2

The simplest thing I can think of here (without knowledge of the code) is to trigger a mouse-leave event when the tab is switched (a leave focus event on the tab?).

Ideally, the link focus state should be at the browser window level, not the tab level, but I suspect this will be harder to implement.

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to comment #6)
> If the link is focused by the user (e.g. by pressing Tab using using '), its
> destination needs to be shown, for accessibility reasons.

What if the link was focused by the user (by pressing 'tab'), and then the URL bar's "preview" area was cleared by hovering & un-hovering another link, and then the user switches tabs & then comes back to this tab?

Currently, the focused link will be "previewed" in the URL bar when the user returns to the tab, even though it wasn't when they left the tab.  That seems broken to me.  (& that's what I filed Bug 600010 on)

Comment 13

[Drew Willcoxon :adw]

There are three times a link can be "indirectly" focused that I can think of:

1. switching tabs
2. going back or forward in history
3. refocusing the browser window

Link targets should not be shown in the location bar for any of these cases, and I'm working on a patch to make it so.

Comment 14

[Drew Willcoxon :adw]

Attachment: patch

This patch fires a "LinkRefocused" chrome event in each of the cases in comment 13.  When XULBrowserWindow in browser.js receives it, it ignores the next setOverLink call.

I wish that the "refocus" information could be imparted somewhere along the stack that ends in setOverLink, e.g., in nsGenericElement::PreHandleEventForLinks and passed up to setOverLink, but I don't see how.  I'm open to better ideas.

Comment 15

[Dão Gottwald [:dao]]

As filed this bug is invalid, as focused links are supposed to display the link target (because pressing enter will activate the link). Consequently this patch introduces some inconsistency, although chances are that users never focusing links with the keyboard won't notice. Mouse users are the majority, but I think I'd still not make this trade-off and, if at all, only take a patch that takes the different link activation scenarios into account.

Comment 16

[Reece H. Dunn]

What about the scenario outlined in comment 10 (Bug 598454 that was marked as a duplicate of this bug)? That is not relating to focused links and is very observable and repeatable.

If this issue is resolved as a WONTFIX, can Bug 598454 be reopened?

Comment 17

[Justin Lebar (not reading bugmail)]

(In reply to comment #15)
> As filed this bug is invalid, as focused links are supposed to display the link
> target (because pressing enter will activate the link).

That's not the current behavior.

If I middle-click a link (thus focusing it) then move my mouse off the link, the link target is no longer displayed in the location bar.  I think this is the right behavior, but I guess we could discuss that in a separate bug.

The bug I'm reporting here is that you can get stuck with the link's target in the location bar if you move the mouse off the link in a separate tab and then tab back to the original tab.

Comment 18

[Drew Willcoxon :adw]

I'm landing this bug one way or another.  Justin (and Daniel and Reece) describes a legitimate annoyance that will affect most users.  When you show link targets in the location bar, you get annoying interactions like this bug, which don't happen when targets are shown in the status bar.

Comment 19

[Drew Willcoxon :adw]

Requesting blocking (final) because we shouldn't ship Firefox 4 like this.

Comment 20

[Drew Willcoxon :adw]

Attachment: skip TriggerLink patch

This patch is an alternate to the previous one.  It modifies nsGenericElement::PreHandleEventForLinks to skip calling nsContentUtils::TriggerLink if the focus event indicates it's a "re-focus".  I changed PreHandleEventForLinks because it's the site closest to the front-end that still has a reference to the focus event that eventually triggers the link target.  I added isRefocus to nsFocusEvent and modified the necessary plumbing to set it to true when a node in a window is focused twice in a row.

Comment 21

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

Comment on attachment 483553 [details] [diff] [review]
skip TriggerLink patch

I think you want aVisitor.mEvent->eventStructType == NS_FOCUS_EVENT rather than the message check.

Comment 22

[Drew Willcoxon :adw]

Attachment: skip TriggerLink patch 2

Neil, comment 20 sketches how this patch works.

Comment 23

[Neil Deakin]

Comment on attachment 483576 [details] [diff] [review]
skip TriggerLink patch 2

>   case NS_FOCUS_CONTENT:
>-    {
>+    if (aVisitor.mEvent->eventStructType != NS_FOCUS_EVENT ||

This could just be an assertion.

>   if (CheckIfFocusable(aContent, aFlags) &&
>       mFocusedWindow == aWindow && mFocusedContent == nsnull) {
>     mFocusedContent = aContent;
>-    aWindow->SetFocusedNode(aContent, focusMethod);
>+
>+    PRBool isRefocus = PR_FALSE;
>+    aWindow->SetFocusedNode(aContent, focusMethod, PR_FALSE, &isRefocus);

You can just compare aContent to aWindow->GetFocusedNode() before it is changed, instead of passing an argument around.

>-                              PRBool aNeedsFocus = PR_FALSE) = 0;
>+                              PRBool aNeedsFocus = PR_FALSE,
>+                              PRBool *aIsRefocus = nsnull) = 0;

You should change the uuid for the interface with this change, but if you did the previous comment, you wouldn't need to change this at all.

Comment 24

[Drew Willcoxon :adw]

Attachment: skip TriggerLink patch 3

Thanks Neil.

(In reply to comment #23)
> >   case NS_FOCUS_CONTENT:
> >-    {
> >+    if (aVisitor.mEvent->eventStructType != NS_FOCUS_EVENT ||
> 
> This could just be an assertion.

Oh, but if the event message is NS_MOUSE_ENTER_SYNTH, isn't it a mouse event and not a focus event?  (The previous case falls through.)

> You can just compare aContent to aWindow->GetFocusedNode() before it is
> changed, instead of passing an argument around.

I used IsEqual, which seems right.  IsEqual requires its arg to be non-null, and aContent will be non-null because CheckIfFocusable in the if-branch guard will return null otherwise.  I took that for granted to save a bit of work but noticed the assignment to sendFocusEvent a couple of statements later doesn't...

> You should change the uuid for the interface with this change, but if you did
> the previous comment, you wouldn't need to change this at all.

Great!

Comment 25

[Neil Deakin]

Comment on attachment 483756 [details] [diff] [review]
skip TriggerLink patch 3

I think you also need to make a change to nsDOMEvent::DuplicatePrivateData(), (where fromRaise is set)

Comment 26

[Drew Willcoxon :adw]

Attachment: skip TriggerLink patch 4

Updates nsDOMEvent::DuplicatePrivateData.  If tryserver says this is OK, I'll try to land tomorrow.

Comment 27

[Drew Willcoxon :adw]

http://hg.mozilla.org/mozilla-central/rev/8e4846fc62e4