856107 - beforehovered and afterhovered attributes are lost after the tab is selected

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: screenshot

In latest UX build on OSX...

STR:
1. Select tab 3
2. Hover tab 1
3. Cycle tabs (to the right) with keyboard so 1 get's selected and then end on 3

You should see the conflicting hover and unselected cases on tab 1 shown in the attachment.

I guess this could probably be duped into #823180 but honestly this feels minor enough to finish that and followup.

Comment 1

[Mike de Boer [:mikedeboer]]

Attachment: mouse hovering events should be handled seperately from tab select/ move code

Comment 2

[Matthew N. [:MattN]]

Comment on attachment 740807 [details] [diff] [review]
mouse hovering events should be handled seperately from tab select/ move code

Review of attachment 740807 [details] [diff] [review]:
-----------------------------------------------------------------

Thank Mike!
This looks okay but I want Adam, the original author, to take a look. Could you also add a test (for beforehovered and afterhovered) in browser/base/content/test/browser_bug585558.js (or a new file) which fails without this patch?

Adam, since this is code you wrote in bug 585558, can you take a look at the proposed patch?

::: browser/base/content/tabbrowser.xml
@@ -3018,5 @@
>            } else {
>              this._afterSelectedTab = visibleTabs[selectedIndex + 1];
>              this._afterSelectedTab.setAttribute("afterselected-visible",
>                                                  "true");
> -            this._afterSelectedTab.removeAttribute("afterhovered");

Shouldn't this also be fixed for beforehovered?

@@ -4268,5 @@
>          if (anonid == "close-button")
>            this.mOverCloseButton = true;
>  
>          let tab = event.target;
> -        if (tab.selected || tab.closing)

Adam, was there a reason we were doing an early return here for the selected tab?

Comment 3

[Mike de Boer [:mikedeboer]]

it *might* also have to be fixed for beforehovered, but I only wanted to remove code that causes this issue, to reduce the potential number of side-effects. The removal of this line fixes the issue. If we want to properly un-mix hover code from select code, a follow-up bug is needed.

I will add a testcase as soon as possible.

Comment 4

[Mike de Boer [:mikedeboer]]

Attachment: unit test coverage

Comment 5

[Adam]

Comment on attachment 740807 [details] [diff] [review]
mouse hovering events should be handled seperately from tab select/ move code

Looks good.

(In reply to Matthew N. [:MattN] from comment #2)
> @@ -4268,5 @@
> >          if (anonid == "close-button")
> >            this.mOverCloseButton = true;
> >  
> >          let tab = event.target;
> > -        if (tab.selected || tab.closing)
> 
> Adam, was there a reason we were doing an early return here for the selected
> tab?

A case of YAGNI/unneeded "optimization."  Quoting myself from bug 585558, comment 10:

> Also updates beforehovered/afterhovered setters to explicitly avoid setting to them on the selected tab.  

The idea was to avoid stacking some of the attributes, where some would take precedent.

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 740807 [details] [diff] [review]
mouse hovering events should be handled seperately from tab select/ move code

(In reply to Mike de Boer [:mikedeboer] from comment #3)
> it *might* also have to be fixed for beforehovered, but I only wanted to
> remove code that causes this issue, to reduce the potential number of
> side-effects.

Fair enough but I would rather have this work consistently for before and after. Also, your change to |mouseover| already affects beforehovered so I think we should fix both here. We should treat this bug as a follow-up issue from bug 585558, not as an Australis-specific fix IMO. Could you also update the test to include beforehovered?

BTW, since bug 585558 landed on m-i/m-c, this would also do the same and bypass the UX branch since it's not Australis-specific. I would probably suggest uplifting this to Firefox 22 where the original patch landed so that developers don't run into this bug with the new attributes.

Thanks

(In reply to Adam [:hobophobe] from comment #5)
> > Also updates beforehovered/afterhovered setters to explicitly avoid setting to them on the selected tab.  
> 
> The idea was to avoid stacking some of the attributes, where some would take
> precedent.

Thanks Adam. I think I'd prefer beforehovered to always be the tab before the hovered one regardless of whether that tab is also selected since we also have beforeselected which I could negate to get the current behaviour.  It seems like you're fine with the proposed change anyways

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 741250 [details] [diff] [review]
unit test coverage

Review of attachment 741250 [details] [diff] [review]:
-----------------------------------------------------------------

This test worked out nicely. f=me while [beforehovered] gets fixed.

::: browser/base/content/test/browser_bug585558.js
@@ +95,5 @@
> +}
> +
> +function test_hoverStatePersistence() {
> +  // test that the afterhovered attribute is still there when a tab is selected
> +  // and then unselected again. This not happening caused bug 856107.

Nit: Capitalize the beginning of the sentence. The second sentence is a bit awkward.

@@ +99,5 @@
> +  // and then unselected again. This not happening caused bug 856107.
> +  gBrowser.selectedTab = gBrowser.tabs[3];
> +  EventUtils.synthesizeMouseAtCenter(gBrowser.tabs[0], { type: "mousemove" });
> +  testAttrib(gBrowser.tabs[1], "afterhovered", true, "Second tab marked afterhovered!");
> +  gBrowser.selectedTab = gBrowser.tabs[0];

Probably wouldn't hurt to also check [afterhovered] after this line

Comment 8

[Mike de Boer [:mikedeboer]]

Attachment: mouse hovering events should be handled seperately from tab select/ move code

carrying over f+ from unusualtears

Comment 9

[Mike de Boer [:mikedeboer]]

Attachment: unit test coverage

Comment 10

[Mike de Boer [:mikedeboer]]

Attachment: unit test coverage

correct patch this time...

Comment 11

[Matthew N. [:MattN]]

Comment on attachment 741805 [details] [diff] [review]
unit test coverage

Test looks good. Please provide a better commit message or fold this with the other patch before pushing.

Comment 12

[Matthew N. [:MattN]]

Comment on attachment 741805 [details] [diff] [review]
unit test coverage

Review of attachment 741805 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/browser_bug585558.js
@@ +94,5 @@
> +  executeSoon(test_hoverStatePersistence);
> +}
> +
> +function test_hoverStatePersistence() {
> +  // Test that the afterhovered and beforehovered attributes are still there when 

Nit: trailing whitespace

@@ +99,5 @@
> +  // a tab is selected and then unselected again. See bug 856107.
> +  gBrowser.selectedTab = gBrowser.tabs[3];
> +  EventUtils.synthesizeMouseAtCenter(gBrowser.tabs[1], { type: "mousemove" });
> +  testAttrib(gBrowser.tabs[0], "beforehovered", true, "First tab marked beforehovered!");
> +  testAttrib(gBrowser.tabs[2], "afterhovered", true, "Third tab marked afterhovered!");

Can you check both attributes on all three tabs with each change? It seems this file is mostly testing for the existence of the correct attributes but not enough testing that the attributes go away when they should.

Comment 13

[Matthew N. [:MattN]]

Comment on attachment 741803 [details] [diff] [review]
mouse hovering events should be handled seperately from tab select/ move code

Review of attachment 741803 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the additional tests in the other patch. Thanks!

Comment 14

[Mike de Boer [:mikedeboer]]

Attachment: unit test coverage

carrying over r+=mnoorenberghe+bmo

Comment 15

[Matthew N. [:MattN]]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=78b6bf078818

I'll push the folded patch to inbound later if it's green.

Comment 16

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/43f7eec8a72d

Thanks

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/43f7eec8a72d