879597 - Using keyboard shortcut to create new tab can cause tab separator to overlap tab curve

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Justin Dolske [:Dolske]]

(Filed from Australis tabs ui-review #1)

OS X (but I suspect x-platform): if you have a selected tab, hover the tab to its left, and use keyboard shortcuts to create more tabs, there is both a hover border and a tab separator to the right of the hovered tab: 

https://dl.dropboxusercontent.com/u/3060457/Screen%20Shot%202013-06-04%20at%2022.26.06%20.png

Comment 1

[Matthew N. [:MattN]]

I suspect this is an issue with bug 585558 but it's possible it's just a selector using those attributes that needs to be fixed.

Comment 2

[:Gijs (he/him)]

This should be feasible for M7.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Removing the items from M7 that do not block landing on m-c.

Comment 4

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: properly track hover state when a tab is added & selected

Matt: please throw this off your queue if you think dolske should review this one instead... I just flagged you cuz bug 856107.

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 775697 [details] [diff] [review]
Patch v1: properly track hover state when a tab is added & selected

if selectedTab = addTab() puts things in a wrong state, that should be fixed internally, as we don't want external tabbrowser API users to call something like _setStateAfterAddTab.

Comment 6

[Mike de Boer [:mikedeboer]]

Comment on attachment 775697 [details] [diff] [review]
Patch v1: properly track hover state when a tab is added & selected

I will file a new patch later with more targeted and robust state correction.

Comment 7

[Mike de Boer [:mikedeboer]]

Attachment: Patch v2: properly track hover state when a tab is added & selected

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 782526 [details] [diff] [review]
Patch v2: properly track hover state when a tab is added & selected

>+            let hoveredTab = this.tabContainer._hoveredTab;
>+
>             if (animate) {
>               mozRequestAnimationFrame(function () {
>                 this.tabContainer._handleTabTelemetryStart(t, aURI);
> 
>                 // kick the animation off
>                 t.setAttribute("fadein", "true");
> 
>                 // This call to adjustTabstrip is redundant but needed so that
>                 // when opening a second tab, the first tab's close buttons
>                 // appears immediately rather than when the transition ends.
>                 if (this.tabs.length - this._removingTabs.length == 2)
>                   this.tabContainer.adjustTabstrip();
>+
>+                if (hoveredTab)
>+                  hoveredTab._leave()._enter();

Why does this need to be called from within the mozRequestAnimationFrame callback in the 'animate' case?

>+      <method name="_enter">
>+        <parameter name="aTab"/>
>+        <body><![CDATA[
>+          aTab = aTab || this;

Umm... why is this a method on the tab if it can act on a different tab? Looks like it should rather be a implemented on the tabContainer?

Comment 9

[Mike de Boer [:mikedeboer]]

(In reply to Dão Gottwald [:dao] from comment #8)
> Umm... why is this a method on the tab if it can act on a different tab?
> Looks like it should rather be a implemented on the tabContainer?

Well, the thing is that I the implementation of these methods were inside the mouseover and mouseout handlers. I simply moved them to be separate methods so they may be called from someplace else. But, you probably noticed that already and would like to see this code moved to the tabContainer?

Comment 10

[Dão Gottwald [:dao]]

(In reply to Mike de Boer [:mikedeboer] from comment #9)
> (In reply to Dão Gottwald [:dao] from comment #8)
> > Umm... why is this a method on the tab if it can act on a different tab?
> > Looks like it should rather be a implemented on the tabContainer?
> 
> Well, the thing is that I the implementation of these methods were inside
> the mouseover and mouseout handlers. I simply moved them to be separate
> methods so they may be called from someplace else.

The mouseover handler could only handle events from the tab it's attached to, not from other tabs, right?

> But, you probably noticed
> that already and would like to see this code moved to the tabContainer?

It depends. Do you actually need the ability to pass an arbitrary tab as a parameter to _enter?

Comment 11

[Mike de Boer [:mikedeboer]]

(In reply to Dão Gottwald [:dao] from comment #10)
> The mouseover handler could only handle events from the tab it's attached
> to, not from other tabs, right?

Indeed.

> It depends. Do you actually need the ability to pass an arbitrary tab as a
> parameter to _enter?

There is no need to have support for a parameter (remnant from an earlier experiment I did). If it's removed, it's okay to keep it like this?

Comment 12

[Dão Gottwald [:dao]]

Ok, so keep them on the tab binding (with the parameter removed), but please rename them to _mouseenter and _mouseleave and drop the 'this' return value.

Comment 13

[Mike de Boer [:mikedeboer]]

(In reply to Dão Gottwald [:dao] from comment #8)
> Why does this need to be called from within the mozRequestAnimationFrame
> callback in the 'animate' case?

Because it didn't work after I did CMD-T for the second time to display the correct state without the separator. We need to cause a reflow AFTER the animation to make sure that IF a tab was hovered before and during addTab(), it'll hide the separator.

This *might* cause a regression of a Talos, if there is any. Dolske mentioned that there is a new Talos test in the making that tests tabstrip perf. This might regress the results of that test further.

Comment 14

[:Gijs (he/him)]

(In reply to Mike de Boer [:mikedeboer] from comment #13)
> (In reply to Dão Gottwald [:dao] from comment #8)
> > Why does this need to be called from within the mozRequestAnimationFrame
> > callback in the 'animate' case?
> 
> Because it didn't work after I did CMD-T for the second time to display the
> correct state without the separator. We need to cause a reflow AFTER the
> animation to make sure that IF a tab was hovered before and during addTab(),
> it'll hide the separator.
> 
> This *might* cause a regression of a Talos, if there is any. Dolske
> mentioned that there is a new Talos test in the making that tests tabstrip
> perf. This might regress the results of that test further.

Sorry if I'm being naive here, but in the following situation:

|a/b\

(b selected)

if you hover a:

/a\/b\

and then hit accel+t to get:

/a\b/c\

We seem to prevent b from having |afterhover| from the beginning, in addition to being selected. Why can't we just change that (and possibly update some CSS so it's specific enough)? It sounds to me like that'd be a simpler fix.

Comment 15

[Mike de Boer [:mikedeboer]]

(In reply to :Gijs Kruitbosch from comment #14)
> We seem to prevent b from having |afterhover| from the beginning, in
> addition to being selected. Why can't we just change that (and possibly
> update some CSS so it's specific enough)? It sounds to me like that'd be a
> simpler fix.

We're not preventing tabs to have these hovering attributes updated, not since bug 856107 was fixed at least.

All the (partly binding-internal) state tracking is happening in the mouseover and mouseout event handlers of individual tabs; setting attributes AND flags for remembering which tab was before- and after-hovered. Since after adding a tab with CMD-T changes the structure of the tabstrip (by adding a node), the state of the attributes and flags I mentioned before are dirty and need to be re-evaluated at the correct time.

Since the implementation of this state tracking code lives inside an event handler, I was not able to not duplicate that code by simply invoking a method to re-evaluate. So what I did was move the code out of the event handlers, to make them accessible from the outside, started keeping track of the currently hovered tab and call these methods when a tab is added to resolve the dirty state.

Comment 16

[Mike de Boer [:mikedeboer]]

Updated STR:

1) Open UX, hover the first, already opened and selected tab.
2) Keep hovering the selected tab and open one or more tabs using CMD-T
3) Notice the tab separator where it shouldn't.

You can also replace 1) with "Hover the currently selected tab." and continue with 2).

Comment 17

[Mike de Boer [:mikedeboer]]

Attachment: Patch v3: properly track hover state when a tab is added & selected

Comment 18

[Dão Gottwald [:dao]]

(In reply to Mike de Boer [:mikedeboer] from comment #13)
> (In reply to Dão Gottwald [:dao] from comment #8)
> > Why does this need to be called from within the mozRequestAnimationFrame
> > callback in the 'animate' case?
> 
> Because it didn't work after I did CMD-T for the second time to display the
> correct state without the separator. We need to cause a reflow AFTER the
> animation to make sure that IF a tab was hovered before and during addTab(),
> it'll hide the separator.

I'm confused. Where in this patch did you "cause a reflow"? And did you mean "after starting the animation" rather than "after the animation"?

Comment 19

[Mike de Boer [:mikedeboer]]

(In reply to Dão Gottwald [:dao] from comment #18)
> I'm confused. Where in this patch did you "cause a reflow"? And did you mean
> "after starting the animation" rather than "after the animation"?

I did indeed mean 'after starting the animation'. The other thing I meant to say is that it needs to have this visual change in the next repaint.

Comment 20

[Dão Gottwald [:dao]]

Comment on attachment 784980 [details] [diff] [review]
Patch v3: properly track hover state when a tab is added & selected

(In reply to Mike de Boer [:mikedeboer] from comment #19)
> (In reply to Dão Gottwald [:dao] from comment #18)
> > I'm confused. Where in this patch did you "cause a reflow"? And did you mean
> > "after starting the animation" rather than "after the animation"?
> 
> I did indeed mean 'after starting the animation'. The other thing I meant to
> say is that it needs to have this visual change in the next repaint.

This actually has nothing to do with reflows or repaints. The reason why calling correctHoveredState directly didn't work (and still doesn't work in the !animate case) is that the newly added tab wouldn't be selected yet at that point. This indicates that addTab isn't the right place for calling _mouseleave and _mouseenter. Maybe this should happen in updateCurrentBrowser?

>       <handler event="mouseover"><![CDATA[
>         let anonid = event.originalTarget.getAttribute("anonid");
>         if (anonid == "close-button")
>           this.mOverCloseButton = true;
> 
>         let tab = event.target;
>         if (tab.closing)
>           return;
...
>+        this._mouseenter.call(tab);
>       ]]></handler>

event.target is always 'this' here, so you can just use the latter and avoid the call() hack.

Comment 21

[Mike de Boer [:mikedeboer]]

(In reply to Dão Gottwald [:dao] from comment #20)
> >       <handler event="mouseover"><![CDATA[
> >         let anonid = event.originalTarget.getAttribute("anonid");
> >         if (anonid == "close-button")
> >           this.mOverCloseButton = true;
> > 
> >         let tab = event.target;
> >         if (tab.closing)
> >           return;
> ...
> >+        this._mouseenter.call(tab);
> >       ]]></handler>
> 
> event.target is always 'this' here, so you can just use the latter and avoid
> the call() hack.

`let tab = event.target;` was added here by hobophobe, but for what reason? Was that necessary because before the handler had `phase="target"`?

I tested the following code:
```js
if (this.closing)
  return;

this._mouseenter();
```
which seems to work fine, without side-effects. Would you be OK with that change?

Additionally, I noticed that `updateCurrentBrowser()` in turn calls `_setPositionalAttributes()`, which is semantically, the best place to move this logic to. I tested this and it has the same effect as the patch you reviewed earlier. What do you think, even though `_setPositionalAttributes()` gets called from multiple areas?

Comment 22

[Dão Gottwald [:dao]]

(In reply to Mike de Boer [:mikedeboer] from comment #21)
> I tested the following code:
> ```js
> if (this.closing)
>   return;
> 
> this._mouseenter();
> ```
> which seems to work fine, without side-effects. Would you be OK with that
> change?

That's basically what I asked you to do, yes.

> Additionally, I noticed that `updateCurrentBrowser()` in turn calls
> `_setPositionalAttributes()`, which is semantically, the best place to move
> this logic to. I tested this and it has the same effect as the patch you
> reviewed earlier. What do you think, even though
> `_setPositionalAttributes()` gets called from multiple areas?

Depends on whether it makes sense to update the beforehovered and afterhovered attributes in all the places where _setPositionalAttributes is called. I suspect it does make sense.

Comment 23

[Mike de Boer [:mikedeboer]]

Attachment: Patch v4: properly track hover state when a tab is added & selected

All nicely contained in one binding! I'm loving it (tm)

Comment 24

[Dão Gottwald [:dao]]

Comment on attachment 809866 [details] [diff] [review]
Patch v4: properly track hover state when a tab is added & selected

>+          let hoveredTab = this._hoveredTab;
>+          if (hoveredTab) {
>+            hoveredTab._mouseleave();
>+            hoveredTab._mouseenter();
>+          }
>         ]]></body>
>       </method>

No need for a variable, just use this._hoveredTab directly in those 3 lines.

>       <method name="_enterNewTab">
>         <body><![CDATA[
>           let visibleTabs = this.tabbrowser.visibleTabs;
>           let candidate = visibleTabs[visibleTabs.length - 1];
>-          if (!candidate.selected) {
>+          if (candidate && !candidate.selected) {
>             this._beforeHoveredTab = candidate;
>             candidate.setAttribute("beforehovered", "true");
>           }
>         ]]></body>
>       </method>

I don't understand this change and it appears to be unrelated to this bug anyway. Please drop it.

>+      <method name="_mouseenter">
>+        <body><![CDATA[
>+          let tabContainer = this.parentNode;
>+          let visibleTabs = tabContainer.tabbrowser.visibleTabs;
>+          let tabIndex = visibleTabs.indexOf(this);
>+          if (tabIndex == 0) {
>+            tabContainer._beforeHoveredTab = null;
>+          } else {
>+            let candidate = visibleTabs[tabIndex - 1];
>+            if (candidate && !candidate.selected) {

Why are you null-checking 'candidate'? It should never be null here.

>+              tabContainer._beforeHoveredTab = candidate;
>+              candidate.setAttribute("beforehovered", "true");
>+            }
>+          }
>+
>+          if (tabIndex == visibleTabs.length - 1) {
>+            tabContainer._afterHoveredTab = null;
>+          } else {
>+            let candidate = visibleTabs[tabIndex + 1];
>+            if (candidate && !candidate.selected) {

ditto

>       <handler event="mouseover"><![CDATA[
>         let anonid = event.originalTarget.getAttribute("anonid");
>         if (anonid == "close-button")
>           this.mOverCloseButton = true;
> 
>-        let tab = event.target;
>-        if (tab.closing)
>+        if (this.closing)
>           return;

This check and return should probably move to _mouseenter.

Comment 25

[Mike de Boer [:mikedeboer]]

(In reply to Dão Gottwald [:dao] from comment #24)]
> >       <method name="_enterNewTab">
> >         <body><![CDATA[
> >           let visibleTabs = this.tabbrowser.visibleTabs;
> >           let candidate = visibleTabs[visibleTabs.length - 1];
> >-          if (!candidate.selected) {
> >+          if (candidate && !candidate.selected) {
> >             this._beforeHoveredTab = candidate;
> >             candidate.setAttribute("beforehovered", "true");
> >           }
> >         ]]></body>
> >       </method>
> 
> I don't understand this change and it appears to be unrelated to this bug
> anyway. Please drop it.

I added these checks because before it threw JS errors in the console while rapidly closing tabs with CTRL+W while hovering a tab somewhere in the middle. So it is possible that it's not set.

Comment 26

[Mike de Boer [:mikedeboer]]

I'm investigating this further.

Comment 27

[Mike de Boer [:mikedeboer]]

(In reply to Dão Gottwald [:dao] from comment #24)
> >+          let hoveredTab = this._hoveredTab;
> >+          if (hoveredTab) {
> >+            hoveredTab._mouseleave();
> >+            hoveredTab._mouseenter();
> >+          }
> >         ]]></body>
> >       </method>
> 
> No need for a variable, just use this._hoveredTab directly in those 3 lines.

I need to use a variable here, because `_mouseleave()` sets the `_hoveredTab` field to null, which would make `this._hoveredTab._mouseenter()` yield a JS error.

Comment 28

[Mike de Boer [:mikedeboer]]

Attachment: Patch v5: properly track hover state when a tab is added & selected

...which culminates to this patch. BTW, I was wrong about it spanning only one binding; it's two bindings.

Comment 29

[Dão Gottwald [:dao]]

Comment on attachment 809891 [details] [diff] [review]
Patch v5: properly track hover state when a tab is added & selected

>+      <method name="_mouseenter">
>+        <body><![CDATA[
>+          if (this.closing)
>+            return;
>+
>+          let tabContainer = this.parentNode;
>+          let visibleTabs = tabContainer.tabbrowser.visibleTabs;
>+          let tabIndex = visibleTabs.indexOf(this);
>+          if (tabIndex == 0) {
>+            tabContainer._beforeHoveredTab = null;
>+          } else {
>+            let candidate = visibleTabs[tabIndex - 1];
>+            if (candidate && !candidate.selected) {

Again, what's the point of the null check? tabIndex should be > 0 here and visibleTabs[tabIndex - 1] should exist.

Comment 30

[Mike de Boer [:mikedeboer]]

Attachment: Patch v6: properly track hover state when a tab is added & selected

Moving `if (this.closing) return;` inside `_mouseenter()` did the trick; 'twas the cause of the candidate being `null` earlier. You were right, again. ;)

Thanks for persisting!

Comment 31

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/projects/ux/rev/10956082ccb7

Comment 32

[Mike de Boer [:mikedeboer]]

Attachment: Patch v2.1: fix mochitest orange

Comment 33

[Dão Gottwald [:dao]]

Comment on attachment 809982 [details] [diff] [review]
Patch v2.1: fix mochitest orange

This seems wrong, we shouldn't let _hoveredTab point to destroyed tabs.

Comment 34

[Mike de Boer [:mikedeboer]]

Attachment: Patch v2.1: fix mochitest orange

Comment 35

[Dão Gottwald [:dao]]

Comment on attachment 810007 [details] [diff] [review]
Patch v2.1: fix mochitest orange

>+            // Invalidate hovered tab state tracking for this closing tab.
>+            if (this.tabContainer._hoveredTab == aTab)
>+              this.tabContainer._hoveredTab = null;

I think calling aTab._mouseleave actually makes more sense... unless something else takes care of removing the before/afterhovered attributes from the sibling tabs?

nit: add blank lines around this block.

Comment 36

[Mike de Boer [:mikedeboer]]

(In reply to Dão Gottwald [:dao] from comment #35)
> I think calling aTab._mouseleave actually makes more sense... unless
> something else takes care of removing the before/afterhovered attributes
> from the sibling tabs?

I'll make it _mouseleave. I didn't do that before, because I didn't want this to needlessly regress Talos (for example). But the other attributes need to be reset indeed, and this doesn't happen in other places.

If perf does matter in the end, we can & should land this on m-c as well (because the bug exists there too, except not visually).

Comment 37

[Mike de Boer [:mikedeboer]]

Attachment: Patch v2.2: fix mochitest orange

Using _mouseleave and fixed nit. Carrying over r=dao.

Comment 38

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/projects/ux/rev/ce4f9fca01b3

Comment 39

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/10956082ccb7
https://hg.mozilla.org/mozilla-central/rev/ce4f9fca01b3