The default bug view has changed. See this FAQ.

urlbar-back-button-clip-path clips the location bar vertically depending on the font size and implied line height

VERIFIED FIXED in Firefox 29

Status

()

Firefox
Theme
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: Cork, Assigned: mikedeboer)

Tracking

(Blocks: 1 bug, {regression, verifyme})

Trunk
Firefox 31
regression, verifyme
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox29 verified, firefox30 verified, firefox31 verified)

Details

(Whiteboard: [Australis:P1])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Created attachment 8398701 [details]
screenshot

After the landing of the keyhold back button the awesomebar top border is missing. It looks like the entire field is shifted up one px.
(Assignee)

Comment 1

3 years ago
Setting n-i for myself, I'll try to reproduce this first asap. Thanks for reporting!
Flags: needinfo?(mdeboer)

Updated

3 years ago
Blocks: 939862
May be related to bug #989701. I see the same on my system (using GNOME 3.12 default theme) but I noticed the whole box moves down as soon as the the forward button is shown. If that is the case, the top border is drawn.

Updated

3 years ago
Depends on: 989701
(Assignee)

Updated

3 years ago
Flags: needinfo?(mdeboer)
Whiteboard: [Australis:P2]
(Assignee)

Comment 3

3 years ago
Cork, since bug 989701 was resolved yesterday, I highly suspect that this has been resolved as well... can you check this for me?

In other words; can you still reproduce this issue with the latest nightly?
Flags: needinfo?(cork)
(Reporter)

Comment 4

3 years ago
I've live modified the css to match and I still have problems with the clip-path being too narrow. So i don't think it will solve it, but I'll test when the nightly comes out in ~an hour.

If i moved the margins around i just clip the bottom or the top border.
(Reporter)

Comment 5

3 years ago
Created attachment 8399971 [details]
2014-04-01

So, yes it is still cropped. clip-path: none !important; fixes the border problem though, so it looks like the svg path is to small.
Flags: needinfo?(cork)

Updated

3 years ago
Duplicate of this bug: 989819
Summary: Top border of awsomebar missing after back/forward theme change → Linux: Top border of awesomebar missing after back/forward theme change
(Assignee)

Updated

3 years ago
Assignee: nobody → mdeboer

Updated

3 years ago
Duplicate of this bug: 989819
(Assignee)

Comment 8

3 years ago
Created attachment 8403975 [details] [diff] [review]
Patch v1: set a fixed line-height so GTK3 doesn't make it larger than the text size

Dão, for some reason, some GTK3 themes decide to add pixels to the line-height for text items of size 14.667 (default size, 1em) so it becomes 21px in practice.

Since I can't use `calc()` for `line-height` (bug 594933), I saw only one way out: hard-code the max value for `line-height` for the clip-path.

What do you think? Do you know of an alternative route we can take?
Attachment #8403975 - Flags: review?(dao)
(Assignee)

Comment 9

3 years ago
BTW, the Fedora GTK theme _does_ show this problem, with font 'Cantarell' (if that matters), but the Ubuntu GTK theme doesn't!
Status: NEW → ASSIGNED
Comment on attachment 8403975 [details] [diff] [review]
Patch v1: set a fixed line-height so GTK3 doesn't make it larger than the text size

This doesn't seem right at all, i.e. it won't handle different text sizes properly.

It seems that the clip path is wrong in assuming that the text field has a certain height...? I don't recall having seen this on Windows. Might be a recent regression from bug 893661?
Attachment #8403975 - Flags: review?(dao) → review-

Updated

3 years ago
Blocks: 893661
(Assignee)

Comment 11

3 years ago
(In reply to Dão Gottwald [:dao] from comment #10)
> It seems that the clip path is wrong in assuming that the text field has a
> certain height...? I don't recall having seen this on Windows. Might be a
> recent regression from bug 893661?

Yes, indeed, it very much seems like that. I thought this might've been a good 'hack' to get the keyhole landed sooner rather than later :/

I'll spend today on getting the clip-path back to its non-fixed size self.
(Assignee)

Comment 12

3 years ago
What I feared kinda happened: http://i.imgur.com/XGuOF8i.png

So a growing urlbar text input makes the urlbar itself grow as well, which makes it larger than the forward button. This is not what we want, presumably, but I'm afraid I don't have a great idea on how to fix this... Do you, Dão?
(Assignee)

Updated

3 years ago
Flags: needinfo?(dao)
(Assignee)

Comment 13

3 years ago
I _could_ use a max-height instead of fiddling with the line-height as I did in my previous patch. Not sure if that would alleviate your concerns...
(Assignee)

Comment 14

3 years ago
Scratch comment 13, this doesn't work - makes the whole navbar layout go bonkers. So, back to option 1: set line-height and live with that for now.
(In reply to Mike de Boer [:mikedeboer] from comment #12)
> What I feared kinda happened: http://i.imgur.com/XGuOF8i.png
> 
> So a growing urlbar text input makes the urlbar itself grow as well, which
> makes it larger than the forward button. This is not what we want,
> presumably, but I'm afraid I don't have a great idea on how to fix this...
> Do you, Dão?

The forward button should grow to match the location bar, but that seems off-topic for this bug.
Flags: needinfo?(dao)
(Assignee)

Comment 16

3 years ago
(In reply to Dão Gottwald [:dao] from comment #15)
> The forward button should grow to match the location bar, but that seems
> off-topic for this bug.

I'd like to reach something actionable here, so what do you suggest we do for this bug? I'd be happy to file a bug re. a growing forward button - but do bear in mind that it's not the `em` value that's different.

On Ubuntu and Fedora (Gnome3 default theme on both), the font-sizes are exactly the same - 1em = 14.667px - but they each use a different sans-serif font-family. "Ubuntu" on Ubuntu - quel surprise! - and "Cantarell" on Fedora (the default Gnome3 font).
It appears that Cantarell occupies more vertical space than strictly necessary - it's a font strictly optimized for small screen legibility - thus increasing the line-height more than the Ubuntu font.

I'm not a font expert enough to say that Cantarell is violating the sans-serif/ font-size vs. line-height rules for GUIs, but to me this does seem to be the case.

Alternatively, setting a fixed line-height of 1.3em also gets the job done, but that's not a whole lot different from 20px.
Flags: needinfo?(dao)
(In reply to Mike de Boer [:mikedeboer] from comment #16)
> (In reply to Dão Gottwald [:dao] from comment #15)
> > The forward button should grow to match the location bar, but that seems
> > off-topic for this bug.
> 
> I'd like to reach something actionable here, so what do you suggest we do
> for this bug?

Fix up the clip path (undo the regression from bug 893661).

> On Ubuntu and Fedora (Gnome3 default theme on both), the font-sizes are
> exactly the same - 1em = 14.667px - but they each use a different sans-serif
> font-family. "Ubuntu" on Ubuntu - quel surprise! - and "Cantarell" on Fedora
> (the default Gnome3 font).
> It appears that Cantarell occupies more vertical space than strictly
> necessary - it's a font strictly optimized for small screen legibility -
> thus increasing the line-height more than the Ubuntu font.

This is a perfectly fine thing to do for a font.

> I'm not a font expert enough to say that Cantarell is violating the
> sans-serif/ font-size vs. line-height rules for GUIs, but to me this does
> seem to be the case.

There's no such rule that I know of.
Flags: needinfo?(dao)
(Assignee)

Comment 18

3 years ago
Alright, on it.
(Assignee)

Comment 19

3 years ago
(In reply to Dão Gottwald [:dao] from comment #17)
> Fix up the clip path (undo the regression from bug 893661).

*sigh*. When I 'fix' the clip-path, I can't work-around the bug I illustrated in the screenshot I made earlier http://i.imgur.com/XGuOF8i.png. The forward-button will always be one pixel too small re. height on Fedora.
Also, note that input fields on the navbar (urlbar and searchbar) are both 1px larger than the toolbarbuttons on Fedora (stock Gnome3), thus not aligning nicely.

This is has little to do with the clip-path anymore and I can't put up a patch that results in the screenshot above.

I'm tempted to move away from this bug as I'm out of ideas/ options.
(In reply to Mike de Boer [:mikedeboer] from comment #19)
> This is has little to do with the clip-path anymore and I can't put up a
> patch that results in the screenshot above.

Yes, you can ;)
The border being cut off and the forward button being too short are separate bugs. The former just hides the latter at the moment and your patch will make it visible again.
(Assignee)

Comment 21

3 years ago
(In reply to Dão Gottwald [:dao] from comment #20)
> Yes, you can ;)

Heh, ok... :)

> The border being cut off and the forward button being too short are separate
> bugs. The former just hides the latter at the moment and your patch will
> make it visible again.

I'm not convinced about that. How do you explain things looking a-ok on Ubuntu? And how do you explain that ALL buttons (not just the forward button) are 1px shorter (see quote below) than the URL bar AND search bar?

(In reply to Mike de Boer [:mikedeboer] from comment #19)
> Also, note that input fields on the navbar (urlbar and searchbar) are both
> 1px larger than the toolbarbuttons on Fedora (stock Gnome3), thus not
> aligning nicely.

Sorry for the spam... It's just that I don't understand things (apparently) and I get the feeling that we're talking past each other... let's fix that!
(In reply to Mike de Boer [:mikedeboer] from comment #21)
> > The border being cut off and the forward button being too short are separate
> > bugs. The former just hides the latter at the moment and your patch will
> > make it visible again.
> 
> I'm not convinced about that. How do you explain things looking a-ok on
> Ubuntu?

It uses a font that occupies less space, as you discovered. The location bar is supposed to grow with larger fonts. It always did that. It still does it today, except that the clip-path is cutting it off as of bug 893661 (which means that this bug affects Windows too if you configure it to use a larger font size).

> And how do you explain that ALL buttons (not just the forward
> button) are 1px shorter (see quote below) than the URL bar AND search bar?

I haven't investigated this -- it seems even more off-topic than the forward button.

Comment 23

3 years ago
(In reply to Dão Gottwald [:dao] from comment #22)
> (In reply to Mike de Boer [:mikedeboer] from comment #21)
> > > The border being cut off and the forward button being too short are separate
> > > bugs. The former just hides the latter at the moment and your patch will
> > > make it visible again.
> > 
> > I'm not convinced about that. How do you explain things looking a-ok on
> > Ubuntu?
> 
> It uses a font that occupies less space, as you discovered. The location bar
> is supposed to grow with larger fonts. It always did that. It still does it
> today, except that the clip-path is cutting it off as of bug 893661 (which
> means that this bug affects Windows too if you configure it to use a larger
> font size).

If this is really the case, do we need to back out bug 893661, at least on beta and/or aurora (or find a way to fix the clip path to not do this)?
(Assignee)

Comment 24

3 years ago
(In reply to :Gijs Kruitbosch from comment #23)
> If this is really the case, do we need to back out bug 893661, at least on
> beta and/or aurora (or find a way to fix the clip path to not do this)?

That is a possibility. Yesterday I sent this email to Stephen:

> It appears that in bug 893661[2], Brandon changed your version of
> 'windows-urlbar-back-button-clip-path’ to something else without any reason
> (I checked and checked again, but no change. And I’m wearing glasses, even).
> However, your pièce de résistance, 'windows-keyhole-forward-clip-path’ did
> require some modification and so he did. But it doesn’t work on Linux anymore.
> 
> On top of that, both clip-paths break the url bar when you set a different
> font-size on both Windows and Linux.
> 
> My question to you: can you redo your 'windows-keyhole-forward-clip-path’,
> which looked like...
> 
> `m 0,0 c .3,.25 .3,.75, 0,1 l 1,0 0,-1 z`
> 
> … so that also scales nicely again?
>
> I’ll be restoring 'windows-urlbar-back-button-clip-path’ to its former glory.

As you can see, I'm asking him to update the `keyhole-forward-clip-path` to be scalable again and I found that the previous `back-button-clip-path` - also authored by Stephen during the previous Toronto workweek - can be restored without any regression.

After the SVG work is done, we're left with the forward button being 1px too small, vertically, and will look like http://i.imgur.com/XGuOF8i.png. The same goes for the other nav-bar buttons, but that's less problematic as the forward button is sealed against the urlbar, thus resulting in a very bad visual glitch.

My gut tells me that this'll end up being a platform/ core bug - a rounding error in font measurements of some sorts. Henceforth, my gut also tells me that due to time-pressure we'll end up using `line-height: 1.3em` for the urlbar and searchbar element.

Or we back-out the Linux keyhole, of course.

Either way, we need to sort all of this out ASAP.
Flags: needinfo?(shorlander)
Flags: needinfo?(madhava)
Whiteboard: [Australis:P2] → [Australis:P1]
(In reply to :Gijs Kruitbosch from comment #23)
> If this is really the case, do we need to back out bug 893661, at least on
> beta and/or aurora (or find a way to fix the clip path to not do this)?

I think for now we can probably do a partial back out of just the clip path change. If this causes some sort of minor visual regression, that can be dealt with separately as a followup to bug 893661.

(In reply to Mike de Boer [:mikedeboer] from comment #24)
> My gut tells me that this'll end up being a platform/ core bug - a rounding
> error in font measurements of some sorts.

No, different fonts can come with different line heights. We just need to deal with that. This should be a no-brainer since we need to deal with different font sizes anyway (but currently don't, due to bug 893661).

> Henceforth, my gut also tells me
> that due to time-pressure we'll end up using `line-height: 1.3em` for the
> urlbar and searchbar element.

That's not going to take care of varying font-sizes.

> Or we back-out the Linux keyhole, of course.

That's not going to take care of the regression on Windows.

Updated

3 years ago
OS: Linux → All
Summary: Linux: Top border of awesomebar missing after back/forward theme change → urlbar-back-button-clip-path clips the location bar vertically depending on the font size and implied line height
(Assignee)

Comment 26

3 years ago
Created attachment 8405337 [details] [diff] [review]
Patch v2: revert clip-path change
Attachment #8403975 - Attachment is obsolete: true
Attachment #8405337 - Flags: review?(dao)
Flags: needinfo?(shorlander)
Flags: needinfo?(madhava)
Comment on attachment 8405337 [details] [diff] [review]
Patch v2: revert clip-path change

Did you see a problem with keyhole-forward-clip-path? Reverting urlbar-back-button-clip-path should be sufficient for this bug, right? Though if it's similarly unclear why keyhole-forward-clip-path was changed in the first place, maybe it's best to revert that as well.
Attachment #8405337 - Flags: review?(dao) → review+

Comment 28

3 years ago
I'm not sure this can safely be partially backed out, because the original patch also changed paddings on the icon, ie the size of the back/fwd buttons. Reverting only the clip path would, I presume, make the clip path and the size of the icon (including padding etc.) mismatch. It's possible this isn't the case, but it'd surprise me.
(Assignee)

Comment 29

3 years ago
(In reply to Dão Gottwald [:dao] from comment #27)
> Did you see a problem with keyhole-forward-clip-path? Reverting
> urlbar-back-button-clip-path should be sufficient for this bug, right?
> Though if it's similarly unclear why keyhole-forward-clip-path was changed
> in the first place, maybe it's best to revert that as well.

I saw a problem with both clip paths, unfortunately, also on Windows with larger font settings.
(Assignee)

Comment 30

3 years ago
Thanks gents!

https://hg.mozilla.org/integration/fx-team/rev/4ea7310a26ee
status-firefox29: --- → affected
status-firefox30: --- → affected
status-firefox31: --- → affected
Whiteboard: [Australis:P1] → [Australis:P1][fixed-in-fx-team]
Filed bug 995300 on the forward button.
https://hg.mozilla.org/mozilla-central/rev/4ea7310a26ee
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox31: affected → fixed
Resolution: --- → FIXED
Whiteboard: [Australis:P1][fixed-in-fx-team] → [Australis:P1]
Target Milestone: --- → Firefox 31
(Assignee)

Comment 33

3 years ago
Comment on attachment 8405337 [details] [diff] [review]
Patch v2: revert clip-path change

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 893661 and brought to light by bug 477948
User impact if declined:

User will not see the bottom border of the urlbar on certain Gnome 3 themes, because it gets clipped by an SVG clip-path. This patch reverts the clip-paths to what they were before 893661.

Testing completed (on m-c, etc.): landed on m-c.
Risk to taking this patch (and alternatives if risky):

there will be a slight visual glitch on Windows, because one clip-path still needs adjustment.

String or IDL/UUID changes made by this patch: n/a.
Attachment #8405337 - Flags: approval-mozilla-beta?
Attachment #8405337 - Flags: approval-mozilla-aurora?
Attachment #8405337 - Flags: approval-mozilla-beta?
Attachment #8405337 - Flags: approval-mozilla-beta+
Attachment #8405337 - Flags: approval-mozilla-aurora?
Attachment #8405337 - Flags: approval-mozilla-aurora+
Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/b885eeedb3cf


Beta: https://hg.mozilla.org/releases/mozilla-beta/rev/f1c211a4714d
status-firefox29: affected → fixed
status-firefox30: affected → fixed
Keywords: verifyme
(Reporter)

Comment 35

3 years ago
Verified fixed in (linux):
https://hg.mozilla.org/mozilla-central/rev/5b6e82e7bbbf
https://hg.mozilla.org/releases/mozilla-aurora/rev/f14047fa8d63

ff29 doesn't have the new button layout in linux so can't test there. (leaving verify me for windows)

Comment 36

3 years ago
(In reply to Cork from comment #35)
> Verified fixed in (linux):
> https://hg.mozilla.org/mozilla-central/rev/5b6e82e7bbbf
> https://hg.mozilla.org/releases/mozilla-aurora/rev/f14047fa8d63
> 
> ff29 doesn't have the new button layout in linux so can't test there.
> (leaving verify me for windows)

Can you try the beta 8 build from https://ftp.mozilla.org/pub/mozilla.org/firefox/releases/29.0b8/ ? This should have the new button layout, AIUI...
Status: RESOLVED → VERIFIED
status-firefox30: fixed → verified
status-firefox31: fixed → verified
Flags: needinfo?(cork)
(Reporter)

Comment 37

3 years ago
Verified fixed in (linux):
https://hg.mozilla.org/releases/mozilla-beta/rev/3a4f085a6398
Flags: needinfo?(cork)
status-firefox29: fixed → verified
You need to log in before you can comment on or make changes to this bug.