tab switching shouldn't occur off a timeout

RESOLVED FIXED in mozilla15

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Gavin, Assigned: ttaubert)

Tracking

(Blocks 1 bug)

Trunk
mozilla15
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Snappy:P1])

Attachments

(2 attachments, 6 obsolete attachments)

Bug 682944 added a timeout between clicks and tab switches to work around a focus styling issue. Unfortunately this has the effect of allowing other work (GC pauses, tab thumbnail capturing, etc.) to fall between the click and the tab switch, which is a rather poor experience for users.

We should back out the patch for bug 682944, and if the problem it was fixing is still present, fix it some other way.
Wrong bug #! I meant bug 302667.
Blocks: 302667

Updated

7 years ago
Whiteboard: [Snappy:P1]
Neil: this fixes this bug by regressing bug 302667. Do you know of an alternate way to solve that bug that doesn't involve the setTimeout?
Attachment #614924 - Flags: feedback?(enndeakin)

Comment 3

7 years ago
Essentially the problem here is that cancelling the mousedown event prevents both the drag and the focusing as both are the default behaviour for mousedown.

Comment 4

7 years ago
(In reply to Neil Deakin from comment #3)
> Essentially the problem here is that cancelling the mousedown event prevents
> both the drag and the focusing as both are the default behaviour for
> mousedown.

so what is the solution?

Comment 5

7 years ago
I suggested to Gavin that the tab switching be done directly without a timeout, leaving only code which sets an attribute that causes -moz-user-focus to be changed within the timeout.

Updated

7 years ago
Attachment #614924 - Flags: feedback?(enndeakin)

Comment 6

7 years ago
(In reply to Neil Deakin from comment #5)
> I suggested to Gavin that the tab switching be done directly without a
> timeout, leaving only code which sets an attribute that causes
> -moz-user-focus to be changed within the timeout.

I don't see anything touching .mozUserFocus in tabbox.xml.
The relevant styling is here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/xul.css#655

We'd probably have to change that to use a different attribute like "normalfocus" ("selected" also impacts a bunch of other styling), and then split the setting of that attribute to the timeout.
Posted patch simple patch (obsolete) — Splinter Review
I took a stab at it and this seems to work fine doing some clicking and dragging. Doesn't regress bug 302667, of course.
Attachment #618062 - Flags: feedback?(gavin.sharp)
Comment on attachment 618062 [details] [diff] [review]
simple patch

Unfortunately it appears that a few addons call selectNewTab directly, so I think we shouldn't change its signature.

I think we also need to ensure that all tabs get "normalfocus" set if the tab selection doesn't go through _selectNewTab (e.g. if someone sets selectedIndex directly). Maybe we could have the tab's "_selected" setter just always set normalfocus on a delay? I'm not sure if that would have any negative side effects...
Attachment #618062 - Flags: feedback?(gavin.sharp) → feedback-
Posted patch patch v2 (obsolete) — Splinter Review
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #9)
> Unfortunately it appears that a few addons call selectNewTab directly, so I
> think we shouldn't change its signature.

Yeah, didn't know that. Boo.

> I think we also need to ensure that all tabs get "normalfocus" set if the
> tab selection doesn't go through _selectNewTab (e.g. if someone sets
> selectedIndex directly). Maybe we could have the tab's "_selected" setter
> just always set normalfocus on a delay? I'm not sure if that would have any
> negative side effects...

I implemented exactly this. Works fine. I have no idea if there could be any negative side-effects though...
Attachment #618062 - Attachment is obsolete: true
Attachment #618079 - Flags: feedback?(gavin.sharp)
Comment on attachment 618079 [details] [diff] [review]
patch v2

Unfortunately this seems to break tabbing to the location bar and then switching back and forth through tabs using the arrow keys (an important accessibility feature).

Maybe we could make this hack more specific to the mousedown case by having the mousedown handler register mouseup/mouseout handlers, and have those set the normalfocus attribute? You'd need to somehow tell the _selected setter to avoid setting normalfocus immediately, which might be ugly, and I'm not sure whether mouseup/mouseout handlers are reliable enough for this to work...
Attachment #618079 - Flags: feedback?(gavin.sharp) → feedback-
Posted patch patch v3 (obsolete) — Splinter Review
Different approach. Basically this just backs out the hack from bug 302667 and removes the workaround for bug 43258 (which is probably regressed by this).

So bug 302667 is fixed again. and I can still use the a11y feature you described. No idea about a better solution for bug 43258, though.
Attachment #618079 - Attachment is obsolete: true
Attachment #620153 - Flags: feedback?(gavin.sharp)
Comment on attachment 620153 [details] [diff] [review]
patch v3

This looks pretty promising. I don't think I care about bug 43258 very much (particularly compared to this bug). I guess you should run this by someone like Neil or smaug?
Attachment #620153 - Flags: feedback?(gavin.sharp) → feedback+
Comment on attachment 620153 [details] [diff] [review]
patch v3

smaug, neil, what do you think about this approach? especially regarding bug 43258?
Attachment #620153 - Flags: feedback?(enndeakin)
Attachment #620153 - Flags: feedback?(bugs)
Comment on attachment 620153 [details] [diff] [review]
patch v3

Uh, looks very risky. Takes some time to review this one.
So, doesn't the patch regress Bug 43258?
...since I don't think we can do that.
(In reply to Olli Pettay [:smaug] from comment #16)
> So, doesn't the patch regress Bug 43258?

Probably. The attached testcases are quite old and I couldn't run them. I wonder if there might be a better solution to bug 43258. I removed lines that said they were a "workaround" for the given problem...
Posted file testcase for bug 43258 (obsolete) —
So, I revived the first testcase of bug 43258. Looks like the testcase isn't fixed in trunk or with this bug's patch applied. If that's true I wonder if we'd actually regress anything or what bug 43258 is supposed to fix?

Comment 20

7 years ago
I thought about doing this, but webkit and IE prevent dragging when the mousedown event is cancelled, so I'm not sure we want to add this incompatibility.

Ideally, there would be a way to indicate that you want a drag to start during a mouse event.
(In reply to Neil Deakin from comment #20)
> I thought about doing this, but webkit and IE prevent dragging when the
> mousedown event is cancelled, so I'm not sure we want to add this
> incompatibility.

Ok, gotcha. Could we maybe only call StopTrackingDragGesture() if the event target is non-XUL?
(In reply to Tim Taubert [:ttaubert] from comment #21)
> Ok, gotcha. Could we maybe only call StopTrackingDragGesture() if the event
> target is non-XUL?

Something like that? Works on my machine™.

>    if (!mCurrentTarget->GetContent()->IsXUL())
>      StopTrackingDragGesture();
Posted patch patch v4 (obsolete) — Splinter Review
This patch calls StopTrackingDragGesture() only if the current target is non-XUL. The only thing it breaks is a test in toolkit/content/tests/chrome/test_tabbox.xul that got introduced with bug 370396.

That's because a mouse click on a tab doesn't really focus the tab itself with event.preventDefault(). So is the tests successfully preventing a regression here or should we adapt it?
Attachment #620153 - Attachment is obsolete: true
Attachment #620153 - Flags: feedback?(enndeakin)
Attachment #620153 - Flags: feedback?(bugs)
Attachment #620353 - Flags: feedback?(enndeakin)
Attachment #620353 - Flags: feedback?(bugs)
Comment on attachment 620353 [details] [diff] [review]
patch v4

Feels horribly hacky. Sorry.
Attachment #620353 - Flags: feedback?(bugs) → feedback-
(In reply to Olli Pettay [:smaug] from comment #24)
> Feels horribly hacky. Sorry.

What's the part that feels hacky? The different behavior between XUL/non-XUL elements?
Yes, the XUL/non-XUL. There can be non-XUL elements in chrome and then behavior would depend
just on the target.
Posted patch patch v2b (obsolete) — Splinter Review
Two steps back. Seems like we can't easily solve this problem the clean way. Not sure there is one.

So... This is a variation of patch v2 but works the other way around. Instead of always restoring normal focus after a timeout we should rather block it on mousedown and unblock it using a timeout. This way we don't need to touch the "selected" property and we still have a11y tab switching with arrow keys.
Attachment #620281 - Attachment is obsolete: true
Attachment #620353 - Attachment is obsolete: true
Attachment #620353 - Flags: feedback?(enndeakin)
Attachment #620516 - Flags: review?(gavin.sharp)
Comment on attachment 620516 [details] [diff] [review]
patch v2b

>diff --git a/toolkit/content/widgets/tabbox.xml b/toolkit/content/widgets/tabbox.xml

>-          // Select new tab after short delay so that PostHandleEvent() doesn't see
>-          // the tab as selected yet, otherwise it will focus the tab for us --
>-          // the CSS for tab has -moz-user-focus: normal only for selected="true".

>+          // Set '-moz-user-focus' to 'ignore' so that the tab doesn't get
>+          // focused when clicking it. After a short timeout we'll reset
>+          // '-moz-user-focus' so that tabs can be selected by keyboard.
>+          this.setAttribute("ignorefocus", "true");
>+          setTimeout(function (tab) tab.removeAttribute("ignorefocus"), 0, this);

This looks good! Much better than my crazy idea :) I wonder whether we should set ignorefocus before calling _selectNewTab - I suppose it probably doesn't matter.

It might be nice to leave the reference to PostHandleEvent(), since that's where we would otherwise end up focusing, AIUI (rather than after handling a click event, as "clicking it" would imply).
Attachment #620516 - Flags: review?(gavin.sharp) → review+
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #28)
> I wonder whether we
> should set ignorefocus before calling _selectNewTab - I suppose it probably
> doesn't matter.

Moved setAttribute() before the _selectNewTab() call.

> It might be nice to leave the reference to PostHandleEvent(), since that's
> where we would otherwise end up focusing, AIUI (rather than after handling a
> click event, as "clicking it" would imply).

Yeah, right. Fixed the comment.

https://hg.mozilla.org/integration/fx-team/rev/c11d28e40534
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Whiteboard: [Snappy:P1] → [Snappy:P1][fixed-in-fx-team]
Target Milestone: --- → mozilla15
Darn, I totally forgot pushing to try. Backed out:

https://hg.mozilla.org/integration/fx-team/rev/73aad6368cf0
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #28)
> I wonder whether we
> should set ignorefocus before calling _selectNewTab - I suppose it probably
> doesn't matter.

Ok, so I found it that it *does* matter. _selectNewTab() passes on focus to the new tab if the old tab was focused. Ideally, we'd check if the tab is focused after calling _selectNewTab() and then we can just skip the 'ignorefocus' part.
Posted patch patch v2cSplinter Review
Calling _selectNewTab() now before we set the 'ignorefocus' attribute so that the focus is passed on if the current tab is focused.

Fixed the toolkit/content/tests/chrome/test_tabbox.xul test to manually focus the tab after the 'ignorefocus' attribute has been removed.
Attachment #620516 - Attachment is obsolete: true
Attachment #621418 - Flags: review?(gavin.sharp)
Whiteboard: [Snappy:P1][fixed-in-fx-team] → [Snappy:P1]
Comment on attachment 621418 [details] [diff] [review]
patch v2c

>diff --git a/toolkit/content/widgets/tabbox.xml b/toolkit/content/widgets/tabbox.xml

>       <handler event="mousedown" button="0">

>+          // Set '-moz-user-focus' to 'ignore' so that PostHandleEvent() doesn't
>+          // see the tab as selected yet, otherwise it will focus the tab for us.

Wouldn't it be more accurate to say:

"... so that PostHandleEvent() can't focus it; we only want tabs to be focusable by the mouse if they are already selected."?

Thanks for your persistence on this one!
Attachment #621418 - Flags: review?(gavin.sharp) → review+
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #34)
> >+          // Set '-moz-user-focus' to 'ignore' so that PostHandleEvent() doesn't
> >+          // see the tab as selected yet, otherwise it will focus the tab for us.
> 
> Wouldn't it be more accurate to say:
> 
> "... so that PostHandleEvent() can't focus it; we only want tabs to be
> focusable by the mouse if they are already selected."?

Yes, that's better. Thanks!
https://hg.mozilla.org/integration/fx-team/rev/d0dc53efd7dd
Whiteboard: [Snappy:P1] → [Snappy:P1][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/d0dc53efd7dd
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Whiteboard: [Snappy:P1][fixed-in-fx-team] → [Snappy:P1]
Blocks: 752918
No longer blocks: 752918

Comment 38

7 years ago
I noticed a similar issue with keyboard shortcuts :
If I open a lot of tabs (especially with flash content) and then do a ctrl+tab+tab+tab+tab+tab+tab there will be short hangs (time to reflow I guess) wich is a really bad UX.

I'm absolutely not a Mozilla dev, just a tester but I think there should be a timeout before the reflow to allocate resources to the good tab.

Should I file a new bug?
(In reply to Jean Claveau from comment #38)
> I noticed a similar issue with keyboard shortcuts :
> If I open a lot of tabs (especially with flash content) and then do a
> ctrl+tab+tab+tab+tab+tab+tab there will be short hangs (time to reflow I
> guess) wich is a really bad UX.
> 
> I'm absolutely not a Mozilla dev, just a tester but I think there should be
> a timeout before the reflow to allocate resources to the good tab.
> 
> Should I file a new bug?

No it's a known issue, we going to work on this in bug 743069 and other related bugs.
Blocks: tabswitch
No longer blocks: 742594
You need to log in before you can comment on or make changes to this bug.