743877 - tab switching shouldn't occur off a timeout

Status: RESOLVED FIXED

(Toolkit :: XUL Widgets, defect)

Description

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

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.

Comment 1

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

Wrong bug #! I meant bug 302667.

Comment 2

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

Attachment: backout of fix for bug 302667

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?

Comment 3

[Neil Deakin]

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

[(dormant account)]

(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

[Neil Deakin]

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.

Comment 6

[(dormant account)]

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

Comment 7

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

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.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Attachment: simple patch

I took a stab at it and this seems to work fine doing some clicking and dragging. Doesn't regress bug 302667, of course.

Comment 9

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

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

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2

(In reply to :Gavin Sharp (use [e-mail] 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...

Comment 11

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

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

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v3

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.

Comment 13

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

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?

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 620153 [details] [diff] [review]
patch v3

smaug, neil, what do you think about this approach? especially regarding bug 43258?

Comment 15

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

Comment on attachment 620153 [details] [diff] [review]
patch v3

Uh, looks very risky. Takes some time to review this one.

Comment 16

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

So, doesn't the patch regress Bug 43258?

Comment 17

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

...since I don't think we can do that.

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

Attachment: testcase for bug 43258

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

[Neil Deakin]

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.

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

(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?

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

(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();

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v4

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?

Comment 24

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

Comment on attachment 620353 [details] [diff] [review]
patch v4

Feels horribly hacky. Sorry.

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

(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?

Comment 26

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

Yes, the XUL/non-XUL. There can be non-XUL elements in chrome and then behavior would depend
just on the target.

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2b

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.

Comment 28

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

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

Comment 29

[Tim Taubert [:ttaubert] (inactive)]

(In reply to :Gavin Sharp (use [e-mail] 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

Comment 30

[Tim Taubert [:ttaubert] (inactive)]

Darn, I totally forgot pushing to try. Backed out:

https://hg.mozilla.org/integration/fx-team/rev/73aad6368cf0

Comment 31

[Tim Taubert [:ttaubert] (inactive)]

(In reply to :Gavin Sharp (use [e-mail] 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.

Comment 32

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2c

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.

Comment 33

[Tim Taubert [:ttaubert] (inactive)]

Passed try this time!

https://tbpl.mozilla.org/?tree=Try&rev=2d178f4bb274

Comment 34

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

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!

Comment 35

[Tim Taubert [:ttaubert] (inactive)]

(In reply to :Gavin Sharp (use [e-mail] 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!

Comment 36

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/d0dc53efd7dd

Comment 37

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/d0dc53efd7dd

Comment 38

[Jean Claveau]

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?

Comment 39

[(dormant account)]

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