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

RESOLVED FIXED in Firefox 28

Status

()

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: jaws, Assigned: MattN)

Tracking

(Blocks 1 bug, {regression})

Trunk
Firefox 28
x86
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Australis:M5])

Attachments

(2 attachments, 7 obsolete attachments)

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
Assignee: nobody → gijskruitbosch+bugs
Whiteboard: [Australis:M3]
This now seems to only happen between the last tab and the new tab button, AFAICT.
Posted patch Patch (obsolete) — Splinter Review
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 .
Attachment #742374 - Flags: review?(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.
Attachment #742374 - Flags: review?(mconley) → review-
Posted patch Make it higher (obsolete) — Splinter Review
Attachment #742374 - Attachment is obsolete: true
Attachment #742478 - Flags: review?(mconley)
Comment on attachment 742478 [details] [diff] [review]
Make it higher

Yep, this does the job, and seems to look right. Thanks Gijs!
Attachment #742478 - Flags: review?(mconley) → review+
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.
Attachment #742478 - Flags: feedback-
I'm having no luck reproducing dolske's issue on my MacBook. Could this be another Retina-only bug?
(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.
Status: NEW → ASSIGNED
Summary: Shrunken tabs resize prematurely when closing tabs if mouse lands between tabs at top of window → Shrunken tabs resize prematurely upon closing if mouse lands between the last tab and new tab button at top of window
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.
Attachment #742604 - Flags: ui-review?(ux-review)
Attachment #742478 - Attachment is obsolete: true
Attachment #742976 - Flags: review?(mnoorenberghe+bmo)
Posted image Screenshot of updated path (obsolete) —
(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)
Attachment #743001 - Flags: ui-review?(ux-review)
Attachment #742604 - Attachment is obsolete: true
Attachment #742604 - Flags: ui-review?(ux-review)
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?
Attachment #742976 - Flags: feedback+
(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.
Posted patch Patch with two paths (obsolete) — Splinter Review
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)
Attachment #742976 - Attachment is obsolete: true
Attachment #743001 - Attachment is obsolete: true
Attachment #742976 - Flags: review?(mnoorenberghe+bmo)
Attachment #743001 - Flags: ui-review?(ux-review)
Attachment #744488 - Flags: review?(mnoorenberghe+bmo)
Comment on attachment 744488 [details] [diff] [review]
Patch with two paths

Stephen is fine with a symmetrical clip-path
Attachment #744488 - Attachment is obsolete: true
Attachment #744488 - Flags: review?(mnoorenberghe+bmo)
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?
Flags: needinfo?(gijskruitbosch+bugs)
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.
Attachment #745881 - Flags: review?(mnoorenberghe+bmo)
Flags: needinfo?(gijskruitbosch+bugs)
Whiteboard: [Australis:M3] → [Australis:M5]
As discussed on IRC. Feel free to land the patch upon review if it's acceptable.
Assignee: gijskruitbosch+bugs → mnoorenberghe+bmo
Attachment #745881 - Attachment is obsolete: true
Attachment #745881 - Flags: review?(mnoorenberghe+bmo)
Attachment #753241 - Flags: review?(gijskruitbosch+bugs)
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.
(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?
(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 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. :-)
Attachment #753241 - Flags: review?(gijskruitbosch+bugs) → review+
Thanks. https://hg.mozilla.org/projects/ux/rev/fad70df06e51
Whiteboard: [Australis:M5] → [Australis:M5][fixed-in-ux]
https://hg.mozilla.org/mozilla-central/rev/fad70df06e51
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M5][fixed-in-ux] → [Australis:M5]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.