Closed
Bug 823237
Opened 12 years ago
Closed 11 years ago
Shrunken tabs resize prematurely upon closing if mouse lands between the last tab and new tab button at top of window
Categories
(Firefox :: Theme, defect)
Tracking
()
RESOLVED
FIXED
Firefox 28
People
(Reporter: jaws, Assigned: MattN)
References
(Blocks 1 open bug)
Details
(Keywords: regression, Whiteboard: [Australis:M5])
Attachments
(2 files, 7 obsolete files)
2.06 KB,
patch
|
Gijs
:
review+
|
Details | Diff | Splinter Review |
21.67 KB,
image/png
|
Details |
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
Updated•12 years ago
|
Assignee: nobody → gijskruitbosch+bugs
Assignee | ||
Updated•12 years ago
|
Whiteboard: [Australis:M3]
Comment 1•12 years ago
|
||
This now seems to only happen between the last tab and the new tab button, AFAICT.
Comment 2•12 years ago
|
||
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 3•12 years ago
|
||
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-
Comment 4•12 years ago
|
||
Attachment #742374 -
Attachment is obsolete: true
Attachment #742478 -
Flags: review?(mconley)
Comment 5•12 years ago
|
||
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+
Assignee | ||
Comment 6•12 years ago
|
||
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-
Comment 7•12 years ago
|
||
I'm having no luck reproducing dolske's issue on my MacBook. Could this be another Retina-only bug?
Assignee | ||
Comment 8•12 years ago
|
||
(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.
Assignee | ||
Updated•12 years ago
|
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
Assignee | ||
Comment 9•12 years ago
|
||
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)
Comment 10•12 years ago
|
||
Attachment #742478 -
Attachment is obsolete: true
Attachment #742976 -
Flags: review?(mnoorenberghe+bmo)
Comment 11•12 years ago
|
||
(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)
Updated•12 years ago
|
Attachment #743001 -
Flags: ui-review?(ux-review)
Updated•12 years ago
|
Attachment #742604 -
Attachment is obsolete: true
Attachment #742604 -
Flags: ui-review?(ux-review)
Assignee | ||
Comment 12•12 years ago
|
||
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+
Comment 13•12 years ago
|
||
(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•12 years ago
|
||
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)
Assignee | ||
Comment 15•12 years ago
|
||
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)
Assignee | ||
Comment 16•12 years ago
|
||
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?
Assignee | ||
Updated•12 years ago
|
Flags: needinfo?(gijskruitbosch+bugs)
Comment 17•12 years ago
|
||
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)
Assignee | ||
Updated•12 years ago
|
Whiteboard: [Australis:M3] → [Australis:M5]
Assignee | ||
Comment 18•12 years ago
|
||
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)
Assignee | ||
Comment 19•12 years ago
|
||
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•12 years ago
|
||
(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?
Assignee | ||
Comment 21•12 years ago
|
||
(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•12 years ago
|
||
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+
Assignee | ||
Comment 23•12 years ago
|
||
Whiteboard: [Australis:M5] → [Australis:M5][fixed-in-ux]
Comment 24•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 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.
Description
•