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

RESOLVED FIXED in Firefox 4.0b7

Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: justin.lebar+bug, Assigned: adw)

Tracking

Trunk
Firefox 4.0b7
Points:
---

Firefox Tracking Flags

(blocking2.0 final+)

Details

Attachments

(2 attachments, 3 obsolete attachments)

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.
(Assignee)

Comment 1

9 years ago
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?
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.
(Assignee)

Updated

9 years ago
Duplicate of this bug: 597163
(Assignee)

Comment 4

9 years ago
(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?
Keywords: uiwanted
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

9 years ago
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.
(Assignee)

Comment 7

9 years ago
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.
(Assignee)

Updated

9 years ago
Blocks: 587908
(Assignee)

Updated

9 years ago
Duplicate of this bug: 598454
(Assignee)

Updated

9 years ago
Duplicate of this bug: 598502

Comment 10

9 years ago
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.
(Assignee)

Updated

9 years ago
Assignee: nobody → adw
Status: NEW → ASSIGNED
Duplicate of this bug: 600010
(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)
(Assignee)

Comment 13

9 years ago
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.
(Assignee)

Comment 14

9 years ago
Posted patch patchSplinter Review
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.
Attachment #478913 - Flags: review?(gavin.sharp)
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

9 years ago
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?
(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.
(Assignee)

Comment 18

9 years ago
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.
(Assignee)

Comment 19

9 years ago
Requesting blocking (final) because we shouldn't ship Firefox 4 like this.
blocking2.0: --- → ?
blocking2.0: ? → final+
(Assignee)

Updated

9 years ago
Attachment #478913 - Flags: review?(gavin.sharp)
(Assignee)

Comment 20

9 years ago
Posted patch skip TriggerLink patch (obsolete) — Splinter Review
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.
Attachment #483553 - Flags: feedback?(gavin.sharp)
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.
Attachment #483553 - Flags: feedback?(gavin.sharp) → feedback+
(Assignee)

Comment 22

9 years ago
Posted patch skip TriggerLink patch 2 (obsolete) — Splinter Review
Neil, comment 20 sketches how this patch works.
Attachment #483553 - Attachment is obsolete: true
Attachment #483576 - Flags: review?(enndeakin)

Comment 23

9 years ago
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.
(Assignee)

Comment 24

9 years ago
Posted patch skip TriggerLink patch 3 (obsolete) — Splinter Review
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!
Attachment #483576 - Attachment is obsolete: true
Attachment #483756 - Flags: review?(enndeakin)
Attachment #483576 - Flags: review?(enndeakin)

Comment 25

9 years ago
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)
Attachment #483756 - Flags: review?(enndeakin) → review+
(Assignee)

Comment 26

9 years ago
Updates nsDOMEvent::DuplicatePrivateData.  If tryserver says this is OK, I'll try to land tomorrow.
Attachment #483756 - Attachment is obsolete: true
(Assignee)

Comment 27

9 years ago
http://hg.mozilla.org/mozilla-central/rev/8e4846fc62e4
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b8
Target Milestone: Firefox 4.0b8 → Firefox 4.0b7
You need to log in before you can comment on or make changes to this bug.