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

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Cork]

Attachment: 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.

Comment 1

[Mike de Boer [:mikedeboer]]

Setting n-i for myself, I'll try to reproduce this first asap. Thanks for reporting!

Comment 2

[Michael Monreal [:micmon]]

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.

Comment 3

[Mike de Boer [:mikedeboer]]

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?

Comment 4

[Cork]

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.

Comment 5

[Cork]

Attachment: 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.

Comment 8

[Mike de Boer [:mikedeboer]]

Attachment: 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?

Comment 9

[Mike de Boer [:mikedeboer]]

BTW, the Fedora GTK theme _does_ show this problem, with font 'Cantarell' (if that matters), but the Ubuntu GTK theme doesn't!

Comment 10

[Dão Gottwald [:dao]]

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?

Comment 11

[Mike de Boer [:mikedeboer]]

(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.

Comment 12

[Mike de Boer [:mikedeboer]]

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?

Comment 13

[Mike de Boer [:mikedeboer]]

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...

Comment 14

[Mike de Boer [:mikedeboer]]

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.

Comment 15

[Dão Gottwald [:dao]]

(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.

Comment 16

[Mike de Boer [:mikedeboer]]

(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.

Comment 17

[Dão Gottwald [: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.

Comment 18

[Mike de Boer [:mikedeboer]]

Alright, on it.

Comment 19

[Mike de Boer [:mikedeboer]]

(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.

Comment 20

[Dão Gottwald [:dao]]

(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.

Comment 21

[Mike de Boer [:mikedeboer]]

(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!

Comment 22

[Dão Gottwald [:dao]]

(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

[:Gijs (he/him)]

(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)?

Comment 24

[Mike de Boer [:mikedeboer]]

(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.

Comment 25

[Dão Gottwald [:dao]]

(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.

Comment 26

[Mike de Boer [:mikedeboer]]

Attachment: Patch v2: revert clip-path change

Comment 27

[Dão Gottwald [:dao]]

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.

Comment 28

[:Gijs (he/him)]

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.

Comment 29

[Mike de Boer [:mikedeboer]]

(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.

Comment 30

[Mike de Boer [:mikedeboer]]

Thanks gents!

https://hg.mozilla.org/integration/fx-team/rev/4ea7310a26ee

Comment 31

[Dão Gottwald [:dao]]

Filed bug 995300 on the forward button.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4ea7310a26ee

Comment 33

[Mike de Boer [:mikedeboer]]

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.

Comment 34

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

Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/b885eeedb3cf


Beta: https://hg.mozilla.org/releases/mozilla-beta/rev/f1c211a4714d

Comment 35

[Cork]

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

[:Gijs (he/him)]

(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...

Comment 37

[Cork]

Verified fixed in (linux):
https://hg.mozilla.org/releases/mozilla-beta/rev/3a4f085a6398