Closed Bug 1459885 Opened 6 years ago Closed 6 years ago

Implement new header style and lay out labels above input fields in the new bookmarks dialog

Categories

(Firefox :: Bookmarks & History, enhancement, P2)

enhancement

Tracking

()

VERIFIED FIXED
Firefox 62
Tracking Status
firefox62 --- verified

People

(Reporter: dao, Assigned: dao)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Instead of:

Name:   [_____________]
Folder: [__________][v]
Tags:   [__________][v]

the new layout looks like this:

Name:
[_____________]

Folder:
[__________][v]

Tags:
[__________][v]
I'll also have to update the header here so the layout still looks balanced.
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Summary: Lay out labels above input fields in the new bookmarks dialog → Implement new header style and lay out labels above input fields in the new bookmarks dialog
This will also affect the library and bookmark properties window. After testing it manually, this seems okay me. It's not necessarily worse than what we have, and the library is supposed to be redesigned anyway.
Attached image macOS Screenshot
Attachment #8976030 - Attachment mime type: text/plain → image/png
Comment on attachment 8975790 [details]
Bug 1459885 - Implement new header style and lay out labels above input fields in the new bookmarks dialog.

https://reviewboard.mozilla.org/r/243998/#review250314

::: browser/base/content/browser.xul:221
(Diff revision 2)
> -      <row id="editBookmarkPanelHeader" align="center" hidden="true">
> -        <vbox align="center">
> -          <image id="editBookmarkPanelStarIcon"/>
> -        </vbox>
> -        <vbox>
> -          <label id="editBookmarkPanelTitle"/>
> +      <label id="editBookmarkPanelTitle"/>

This can use the .panel-header class.
Comparing this to the mockup, I can see a few issues related to this specific change:
1. The title in the mockup is bold, I think the current non-bold title doesn't look great.
2. The title is not centered afaict

Since this work will happen in chunks, I think each chunk should have an "acceptable" design, so we don't have to rush changes to make a merge. Those 2 small title tweaks would help a lot, imo.
I forgot, I tested this on win10, and I wonder if the issue is due to bitrotting...
For 1. It's bold when I tested on macOS and consistent with other subview headers
For 2. this margin [0] needs to be factored out to only apply to `.panel-header > .subviewbutton-back + label`


[0]: https://searchfox.org/mozilla-central/source/browser/themes/shared/customizableui/panelUI.inc.css#1509
Comment on attachment 8975790 [details]
Bug 1459885 - Implement new header style and lay out labels above input fields in the new bookmarks dialog.

https://reviewboard.mozilla.org/r/243998/#review251726

could you please unbitrot the patch and fix title alignment?

::: browser/themes/linux/jar.mn:35
(Diff revision 3)
>    skin/classic/browser/notification-icons/geo-detailed.svg (notification-icons/geo-detailed.svg)
>    skin/classic/browser/notification-icons/geo.svg          (notification-icons/geo.svg)
>    skin/classic/browser/places/allBookmarks.png        (places/allBookmarks.png)
>    skin/classic/browser/places/bookmarksMenu.png       (places/bookmarksMenu.png)
>    skin/classic/browser/places/bookmarksToolbar.png    (places/bookmarksToolbar.png)
> -* skin/classic/browser/places/editBookmark.css (places/editBookmark.css)
> +  skin/classic/browser/places/editBookmark.css (places/editBookmark.css)

while here could you please align at the second column?
Attachment #8975790 - Flags: review?(mak77)
Comment on attachment 8975790 [details]
Bug 1459885 - Implement new header style and lay out labels above input fields in the new bookmarks dialog.

https://reviewboard.mozilla.org/r/243998/#review252080

::: browser/themes/shared/customizableui/panelUI.inc.css:1506
(Diff revision 4)
>  }
>  
>  .panel-header > label {
>    flex: auto;
>    font-size: 13px;
>    font-weight: 500;

Ok, it is not bold at all for me yet, it starts looking bold at font-weight: 501. And at that point all the title labels in the Library panel look much better.
Indeed I never understood why those labels were looking so poorly...

Could we evaluate bumping this up to 550?
Attachment #8975790 - Flags: review?(mak77) → review+
well, or 600 since looks like the spec is about hundreds. But maybe it's just the Windows 10 font, that fallbacks to 400.
(In reply to Marco Bonardo [::mak] from comment #15)
> >  .panel-header > label {
> >    flex: auto;
> >    font-size: 13px;
> >    font-weight: 500;
> 
> Ok, it is not bold at all for me yet, it starts looking bold at font-weight:
> 501. And at that point all the title labels in the Library panel look much
> better.
> Indeed I never understood why those labels were looking so poorly...
> 
> Could we evaluate bumping this up to 550?

I don't think we want to do this here since it will affect all panels. I'll file a new bug.
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/11f29bd57a65
Implement new header style and lay out labels above input fields in the new bookmarks dialog. r=mak
Blocks: 1463685
https://hg.mozilla.org/mozilla-central/rev/11f29bd57a65
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Depends on: 1471546
Managed to confirm the old design using Firefox 60.0.
Have checked on macOS 10.9.5, win10x64, Ubuntu 16.04.LTS - with Firefox 62.0b8 and 63.0a1 (2018-07-16) and can confirm the layout is as mentioned in comment 0.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
No longer depends on: 1471546
Regressions: 1471546
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: