823237 - Shrunken tabs resize prematurely upon closing if mouse lands between the last tab and new tab button at top of window

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

STR:
Use a build with the Australis tabs
Maximize the window
Open enough tabs that the tab size shrinks
Move the mouse to the vertical center of a tab
Middle click on the tab to close the tab
->  Close enough of these tabs such that moving the
    mouse out of the tabbar would cause them to grow in size
Move the mouse to the top of the screen and between tabs

Expected:
The tabs should not resize since the cursor is still over the tabbar

Actual:
The tabs resize since the cursor is not on top of a tab

Comment 1

[:Gijs (he/him)]

This now seems to only happen between the last tab and the new tab button, AFAICT.

Comment 2

[:Gijs (he/him)]

Attachment: Patch

So the problem seems to be that the clip paths that were updated in bug 856749 were only for the tabs, and not for the new tab button (which has a separate clip path because it only has 1 element rather than the two side elements). This is an update for the clip path of the new tab button.

You can sort of see the problem outlined here happen on OS X as well if you select any but the last tab, and hover between the last tab and the new tab button - there's a gap there, where neither the tab nor the button gets hover. This change to the clip path should fix that without adversely affecting clicking on the right edge of the last tab (ie, the new tab button shouldn't overlap the last tab).

It works in my testing on OS X, I've fired off a try build to see if this really fixes the win32 issues: https://tbpl.mozilla.org/?tree=Try&rev=fee55d609946 .

Comment 3

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

Comment on attachment 742374 [details] [diff] [review]
Patch

I agree that it's only the new-tab button that causes this, but I just tested this patch, and the behaviour still persists in Windows 7.

Comment 4

[:Gijs (he/him)]

Attachment: Make it higher

Comment 5

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

Comment on attachment 742478 [details] [diff] [review]
Make it higher

Yep, this does the job, and seems to look right. Thanks Gijs!

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 742478 [details] [diff] [review]
Make it higher

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

Dolske was able to repro this yesterday between a background and selected tab on OS X too.

Comment 7

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

I'm having no luck reproducing dolske's issue on my MacBook. Could this be another Retina-only bug?

Comment 8

[Matthew N. [:MattN]]

(In reply to Matthew N. [:MattN] from comment #6)
> Dolske was able to repro this yesterday between a background and selected
> tab on OS X too.

Sorry, I think he might not have been in the delayed animation phase when we were looking at the pointer-events and his cursor may have been on the selected tab side of the gap which means it's correct that there is no hover feedback.

RTL does need to be addressed though.

Comment 9

[Matthew N. [:MattN]]

Attachment: Screenshot of clip-path from attachment 742478

UX team: What should the clip-path look like on the new tab button when there is no overflow? Having pointer-events on the side adjacent to the tabs prevent the tabs from resizing prematurely upon closing.  Should we make the top of the clip-path extend horizontally to the middle of the curve like we do on tabs?

Gijs: If that screenshot is actually what the patch uses, can you make the top-left corner use straight line segments with a right angle.

Comment 10

[:Gijs (he/him)]

Attachment: Make path straight on both sides to deal with RTL

Comment 11

[:Gijs (he/him)]

Attachment: Screenshot of updated path

(if we think having it straight on both sides is a serious problem, we could use a separate clip-path for the RTL case and only have it straight on one side, but I don't think that that is worth the added complexity)

Comment 12

[Matthew N. [:MattN]]

Comment on attachment 742976 [details] [diff] [review]
Make path straight on both sides to deal with RTL

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

::: browser/base/content/browser.xul
@@ +1202,5 @@
>      <svg:clipPath id="tab-curve-clip-path-end" clipPathUnits="userSpaceOnUse">
>        <svg:path d="M -1,2 C 12.951,2.104,14.492,11.669,15.489,17 c +1.565,8.376,+4.276,13,+13,13 L 30,31 H -1 V 2 z"/>
>      </svg:clipPath>
>      <svg:clipPath id="tab-clip-path-outer" clipPathUnits="userSpaceOnUse">
> +      <svg:path d="M 13.1,-1 12.93,14.6 C 12.77,18.86 11.21,22.7 9.18,25.3 7.1,27.7 3.8,28 0.8,28 c -0.4,0.6 -2.09,1.11 -1.46,1.8 l 2.45,2.72 61.6,0 2.59,-3.06 C 65.15,28.96 64.57,27.8 63.5,28 60.13,28.13 56.33,27.03 54.74,23.78 53.295,21.11 52.24,19.13 52.17,15.16 L 51.89,-1 z"/>

How did you choose the point at which the curve switches to become a vertical line? Did you just use the existing point in the curve or did you ensure that the clip-path lines up with the tab separator? Did you make sure that the rectangular top portion doesn't overlap the adjacent tab for a few pixels?

Comment 13

[:Gijs (he/him)]

(In reply to Matthew N. [:MattN] from comment #12)
> Comment on attachment 742976 [details] [diff] [review]
> Make path straight on both sides to deal with RTL
> 
> Review of attachment 742976 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser.xul
> @@ +1202,5 @@
> >      <svg:clipPath id="tab-curve-clip-path-end" clipPathUnits="userSpaceOnUse">
> >        <svg:path d="M -1,2 C 12.951,2.104,14.492,11.669,15.489,17 c +1.565,8.376,+4.276,13,+13,13 L 30,31 H -1 V 2 z"/>
> >      </svg:clipPath>
> >      <svg:clipPath id="tab-clip-path-outer" clipPathUnits="userSpaceOnUse">
> > +      <svg:path d="M 13.1,-1 12.93,14.6 C 12.77,18.86 11.21,22.7 9.18,25.3 7.1,27.7 3.8,28 0.8,28 c -0.4,0.6 -2.09,1.11 -1.46,1.8 l 2.45,2.72 61.6,0 2.59,-3.06 C 65.15,28.96 64.57,27.8 63.5,28 60.13,28.13 56.33,27.03 54.74,23.78 53.295,21.11 52.24,19.13 52.17,15.16 L 51.89,-1 z"/>
> 
> How did you choose the point at which the curve switches to become a
> vertical line? Did you just use the existing point in the curve or did you
> ensure that the clip-path lines up with the tab separator? Did you make sure
> that the rectangular top portion doesn't overlap the adjacent tab for a few
> pixels?

I attempted to do the last of those. However, I believe shorlander is looking at what to do here, so I'd like to wait for that. In any case, I suppose ideally we'd have separate paths for the LTR/RTL cases so that the clip path matches the curve on the side not adjacent to the tab.

Comment 14

[:Gijs (he/him)]

Attachment: Patch with two paths

Here's a JSBin with these two paths and the original for comparison, scaled up 5x: http://jsbin.com/egunom/2/edit

I've made these touch the tab separator as narrowly as I can, and extend to the top of the browser window. They'll also work at 200% DPI.

(note that there's some interesting visual illusion going on that makes them seem of different sizes; if you use the 'top' to overlay them, you'll see that they're really the same size)

Comment 15

[Matthew N. [:MattN]]

Comment on attachment 744488 [details] [diff] [review]
Patch with two paths

Stephen is fine with a symmetrical clip-path

Comment 16

[Matthew N. [:MattN]]

Comment on attachment 742976 [details] [diff] [review]
Make path straight on both sides to deal with RTL

Gijs, could you address comment 12, rebase on top of bug 850918 and make the clip-path scale?

Comment 17

[:Gijs (he/him)]

Attachment: Patch with straight, scaled paths

Aligned this to match up with the tabs and not overlap. Note that the bottom left still overlaps, but that's not a regression and if we want to fix that IMHO it should be done separately.

Comment 18

[Matthew N. [:MattN]]

Attachment: MN v.1 Rectangular top

As discussed on IRC. Feel free to land the patch upon review if it's acceptable.

Comment 19

[Matthew N. [:MattN]]

Attachment: MN v.1 Screenshot of forced :hover

The straight segment begins at 14.5px from the left because 15px (half curve width) left s small gap in pointer-events (perhaps due to rounding?). I tested for gaps in pointer-events with Windows MouseKeys and didn't find any problems.

Comment 20

[:Gijs (he/him)]

(In reply to Matthew N. [:MattN] from comment #19)
> Created attachment 753242 [details]
> MN v.1 Screenshot of forced :hover
> 
> The straight segment begins at 14.5px from the left because 15px (half curve
> width) left s small gap in pointer-events (perhaps due to rounding?). I
> tested for gaps in pointer-events with Windows MouseKeys and didn't find any
> problems.

I'm confused, because when compared to the previous clippath, it looks like the left curve was moved: http://jsbin.com/egunom/4/edit

Was that intentional, or am I missing something?

Comment 21

[Matthew N. [:MattN]]

(In reply to :Gijs Kruitbosch from comment #20)
> (In reply to Matthew N. [:MattN] from comment #19)
> > Created attachment 753242 [details]
> > MN v.1 Screenshot of forced :hover
> > 
> > The straight segment begins at 14.5px from the left because 15px (half curve
> > width) left s small gap in pointer-events (perhaps due to rounding?). I
> > tested for gaps in pointer-events with Windows MouseKeys and didn't find any
> > problems.
> 
> I'm confused, because when compared to the previous clippath, it looks like
> the left curve was moved: http://jsbin.com/egunom/4/edit
> 
> Was that intentional, or am I missing something?

That was intentional and what I was referring to when I told you on IRC that the old clip-path wasn't symmetrical. IIRC, your clip-path also appeared shifted in the same way when I compared. I'm fairly confident my clip-path is has the rectangles starting at 14.5px from both sides. In other words, I think the old clip-path was misaligned/wrong. I remembered the width was 65px at one point in my patch queue so that could explain why it's shifted.

FTR, the coordinates were the following with userSpaceOnUse:
m 14.5,0 0,13.21875 c -1.031556,3.837524 -1.325,7.898438 -3.25,11.5625 C 9.66,27.99375 5.87,29.13 2.5,29 L 0,31 66,31 63.5,29 c -3.37,0.13 -7.16,-0.99875 -8.75,-4.21875 -1.925,-3.57 -2.205469,-7.705869 -3.25,-11.5625 L 51.5,0 z

Note the 14.5 and 51.5 (66-14.5) x-coordinates.

Comment 22

[:Gijs (he/him)]

Comment on attachment 753241 [details] [diff] [review]
MN v.1 Rectangular top

(In reply to Matthew N. [:MattN] from comment #21)
> (In reply to :Gijs Kruitbosch from comment #20)
> > (In reply to Matthew N. [:MattN] from comment #19)
> > > Created attachment 753242 [details]
> > > MN v.1 Screenshot of forced :hover
> > > 
> > > The straight segment begins at 14.5px from the left because 15px (half curve
> > > width) left s small gap in pointer-events (perhaps due to rounding?). I
> > > tested for gaps in pointer-events with Windows MouseKeys and didn't find any
> > > problems.
> > 
> > I'm confused, because when compared to the previous clippath, it looks like
> > the left curve was moved: http://jsbin.com/egunom/4/edit
> > 
> > Was that intentional, or am I missing something?
> 
> That was intentional and what I was referring to when I told you on IRC that
> the old clip-path wasn't symmetrical. IIRC, your clip-path also appeared
> shifted in the same way when I compared. I'm fairly confident my clip-path
> is has the rectangles starting at 14.5px from both sides. In other words, I
> think the old clip-path was misaligned/wrong. I remembered the width was
> 65px at one point in my patch queue so that could explain why it's shifted.
> 
> FTR, the coordinates were the following with userSpaceOnUse:
> m 14.5,0 0,13.21875 c -1.031556,3.837524 -1.325,7.898438 -3.25,11.5625 C
> 9.66,27.99375 5.87,29.13 2.5,29 L 0,31 66,31 63.5,29 c -3.37,0.13
> -7.16,-0.99875 -8.75,-4.21875 -1.925,-3.57 -2.205469,-7.705869
> -3.25,-11.5625 L 51.5,0 z
> 
> Note the 14.5 and 51.5 (66-14.5) x-coordinates.

Ah, makes sense! In which case, r=me. Thanks also for the screenshot, that is helpful. :-)

Comment 23

[Matthew N. [:MattN]]

Thanks. https://hg.mozilla.org/projects/ux/rev/fad70df06e51

Comment 24

[:Gijs (he/him)]

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