1388794 - Implement correct height of photon bookmarks toolbar for all density options

Status: RESOLVED WORKSFORME

(Firefox :: Theme, defect)

Description

[Edo Karabas]

Attachment: compact bookmark toolbar 200%.png

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170809100326

Steps to reproduce:

When comparing current Nightly and what I'm seeing here:
http://design.firefox.com/people/shorlander/photon/Mockups/windows-10.html
The bookmarks toolbar is currently not following Mockup in any UI density (Compact, Normal, Touch).
As far as I see it correcly, 
comparing to current Nightly Compact UI, bookmarks toolbar height should be 4px lower,
comparing to current Nightly Normal UI, bookmarks toolbar height should be 3px lower,
comparing to current Nightly Touch UI, bookmarks toolbar height should be 4px bigger,
For compact mode example see attachment.

Comment 1

[Dão Gottwald [:dao]]

Bug 1369415 should help with this. Is it better with this build? https://queue.taskcluster.net/v1/task/YIUhlNyNTj2cz-18EVTCHQ/runs/0/artifacts/public/build/target.zip

Comment 2

[Edo Karabas]

better but not completely, still 2px AFAISC
400% for compact, didn't check another densities
https://s1.postimg.org/784wg1djz/400_test.png

Btw I just noticed that bottom border should have a slightly different color. There are some specs for this probably.

Comment 3

[Edo Karabas]

Comparing to current Nightly your patch basically cut -2px for Compact, -2px for Normal and -2px also for Touch.
There is needed another 2px cut for compact to match a Mockup, 1px cut for normal to match a Mockup and now 6px add for Touch to match a Mockup.

Comment 5

[Marco Bonardo [:mak]]

Can we please re-evaluate the priority of this in spite of the recent changes to the toolbar?
The current toolbar buttons click area, at least on Windows 10, is sub par, it requires aiming, and this is on the normal density.

Comment 6

[Marco Bonardo [:mak]]

To be clear, the problem is the buttons active area, they need some vertical padding.

Comment 7

[Johann Hofmann [:johannh]]

I don't think what you're seeing is missing space in the bookmarks toolbar, but rather that the navbar doesn't have correct vertical spacing. In any case, the dimensions of the bookmark items seem according to spec to me, the height (including padding) is 18px in the spec, mockup and in our implementation.

I'm leaning towards WONTFIX for this.

Comment 8

[Edo Karabas]

Yes, active are for buttons is a bit smaller, because bugs like bug 1391593 landed. Perhaps there needs to be implemented a condition for buttons to span the full height of the toolbar.
The size of the toolbar is about right, but not completely per spec. I will do a comparison mockup vs todays Nightly for all density options, but it will be only a few px up or down, like I said in the https://bugzilla.mozilla.org/show_bug.cgi?id=1388794#c3

Comment 9

[Johann Hofmann [:johannh]]

(In reply to Eddward from comment #8)
> Yes, active are for buttons is a bit smaller, because bugs like bug 1391593
> landed. Perhaps there needs to be implemented a condition for buttons to
> span the full height of the toolbar.
> The size of the toolbar is about right, but not completely per spec. I will
> do a comparison mockup vs todays Nightly for all density options, but it
> will be only a few px up or down, like I said in the
> https://bugzilla.mozilla.org/show_bug.cgi?id=1388794#c3

Ok, my apologies, it seems like bug 1391593 indeed made the bookmarks items lose their vertical padding on Windows. I'll file a bug.

I still don't think the vertical dimensions of the bookmarks toolbar are incorrect. I think what you're seeing is different/wrong nav-bar height. Note that compact mode does not have a different bookmark toolbar height.

Comment 10

[Edo Karabas]

So looks like at least in Normal density the height of the toolbar needs some care. Aligned with the URL bar the height of the bookmarks toolbar is currently 3px less than mockup. So +3px would match the spec. For touch there is needed 1px, but it seems like irrelevant since everything is already big enough.

Comment 11

[Edo Karabas]

Attachment: normal 100%.png

Comment 12

[Edo Karabas]

Attachment: normal 300%.png

Comment 13

[Edo Karabas]

(In reply to Johann Hofmann [:johannh] from comment #9)
> (In reply to Eddward from comment #8)
> > Yes, active are for buttons is a bit smaller, because bugs like bug 1391593
> > landed. Perhaps there needs to be implemented a condition for buttons to
> > span the full height of the toolbar.
> > The size of the toolbar is about right, but not completely per spec. I will
> > do a comparison mockup vs todays Nightly for all density options, but it
> > will be only a few px up or down, like I said in the
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1388794#c3
> 
> Ok, my apologies, it seems like bug 1391593 indeed made the bookmarks items
> lose their vertical padding on Windows. I'll file a bug.
> 
> I still don't think the vertical dimensions of the bookmarks toolbar are
> incorrect. I think what you're seeing is different/wrong nav-bar height.
> Note that compact mode does not have a different bookmark toolbar height.

I think for bookmarks it's okay (since it's as per spec?), but if you can exclude buttons for that rule it could be helpful for people.

Comment 14

[Johann Hofmann [:johannh]]

I filed bug 1401523 for the padding issue, thanks for reporting this!

Comment 15

[Edo Karabas]

(In reply to Johann Hofmann [:johannh] from comment #14)
> I filed bug 1401523 for the padding issue, thanks for reporting this!

no problem :)

Regarding this:
Note that compact mode does not have a different bookmark toolbar height.

Well that's weird. Mockup is saying there is (has to be) a difference. 2px more for Normal (2px less for Compact).

Comment 16

[Edo Karabas]

Attachment: mockup compact vs normal 300%.png

Comment 17

[Johann Hofmann [:johannh]]

(In reply to Eddward from comment #15)
> (In reply to Johann Hofmann [:johannh] from comment #14)
> > I filed bug 1401523 for the padding issue, thanks for reporting this!
> 
> no problem :)
> 
> Regarding this:
> Note that compact mode does not have a different bookmark toolbar height.
> 
> Well that's weird. Mockup is saying there is (has to be) a difference. 2px
> more for Normal (2px less for Compact).

I'm quite sure that what you mean by bookmark toolbar height is bottom padding of the nav-bar. Your screenshots are a little misleading. Try toggling compact mode on and off and watching the dimensions of the two toolbars.

Comment 18

[Edo Karabas]

Ok, understood. If not Bookmarks toolbar then something else is not quite right, but anyway, the height as of now has a much better dimensions as it was in time I filed the bug. I have compared Nightly from August 9th and Todays. 
I have filed this bug mainly because in Compact mode the height was unneccesary bigger, right now it seems okay, for touch is also okay, so if someone needs to reevaluate for normal density or something than can file another bug and this can be closed as wontfix or resolved worksforme.