Closed Bug 856107 Opened 7 years ago Closed 7 years ago

beforehovered and afterhovered attributes are lost after the tab is selected

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 23
Tracking Status
firefox22 --- affected

People

(Reporter: zpao, Assigned: mikedeboer)

References

Details

(Whiteboard: [Australis:M3])

Attachments

(3 files, 4 obsolete files)

Attached image 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.
Whiteboard: [Australis:M3]
Assignee: mnoorenberghe+bmo → mdeboer
Status: NEW → ASSIGNED
Attachment #740807 - Attachment is patch: true
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?
Attachment #740807 - Flags: feedback?(unusualtears)
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.
Attached patch unit test coverage (obsolete) — Splinter Review
Attachment #741250 - Flags: review?(mnoorenberghe+bmo)
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.
Attachment #740807 - Flags: feedback?(unusualtears) → feedback+
OS: Mac OS X → All
Hardware: x86 → All
Summary: Australis tabs have hover/unselected conflict → beforehovered and afterhovered attributes are lost after the tab is selected
Component: Theme → Tabbed Browser
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
Attachment #740807 - Flags: review?(mnoorenberghe+bmo) → feedback+
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
Attachment #741250 - Flags: review?(mnoorenberghe+bmo) → feedback+
carrying over f+ from unusualtears
Attachment #740807 - Attachment is obsolete: true
Attachment #741803 - Flags: review?(mnoorenberghe+bmo)
Attachment #741803 - Flags: feedback+
Attached patch unit test coverage (obsolete) — Splinter Review
Attachment #741250 - Attachment is obsolete: true
Attachment #741804 - Flags: review?(mnoorenberghe+bmo)
Attached patch unit test coverage (obsolete) — Splinter Review
correct patch this time...
Attachment #741804 - Attachment is obsolete: true
Attachment #741804 - Flags: review?(mnoorenberghe+bmo)
Attachment #741805 - Flags: review?(mnoorenberghe+bmo)
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.
Attachment #741805 - Flags: review?(mnoorenberghe+bmo) → review+
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 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!
Attachment #741803 - Flags: review?(mnoorenberghe+bmo) → review+
carrying over r+=mnoorenberghe+bmo
Attachment #741805 - Attachment is obsolete: true
Attachment #744087 - Flags: review+
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=78b6bf078818

I'll push the folded patch to inbound later if it's green.
https://hg.mozilla.org/mozilla-central/rev/43f7eec8a72d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
You need to log in before you can comment on or make changes to this bug.