Closed Bug 1383009 Opened 7 years ago Closed 7 years ago

Add flexible spaces around the URL and search bar by default and replace the bookmarks button with the library one

Categories

(Firefox :: Toolbars and Customization, enhancement, P1)

Product:
Firefox
Component:
Toolbars and Customization
Version:
55 Branch
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 57
Iteration:
57.1 - Aug 15
Tracking Flags:
Tracking Status
firefox56 --- wontfix
firefox57 --- verified

People

(Reporter: Gijs, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-structure])

Attachments

(3 files)

Bug 1383009 - add flexible spacers and library button to the navbar by default,
59 bytes, text/x-review-board-request
jaws
: review+
Details
Bug 1383009 - fix tests, BrowserUITelemetry and pocket to deal with the changes to the navbar,
59 bytes, text/x-review-board-request
jaws
: review+
Details
Bug 1383009 - fix logic for url bar autocomplete item positioning to allow it to position items in more cases,
59 bytes, text/x-review-board-request
adw
: review+
Details 
The photon designs show these spaces around the url/search bars.

I'm less clear on whether we need to update existing profiles to have them, too, or not. Aaron?
Flags: qe-verify+
(In reply to :Gijs from comment #0)
> The photon designs show these spaces around the url/search bars.
> 
> I'm less clear on whether we need to update existing profiles to have them,
> too, or not. Aaron?

With needinfo this time...
Flags: needinfo?(abenson)
Remember that by design the location bar popup contents will be limited to the urlbar input field, so if we add spaces also when the search bar is visible, we'll limit the contents of the location bar popup quite a bit.
QA Contact: gwimberly
Blocks: 1387512
Can Confirm that flexible spacers to the left of the URL bar, and To the right side of the URL bar and/or the Search Bar (if present) is a requirement.
Flags: needinfo?(abenson)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 57.1 - Aug 15
Priority: P2 → P1
Blocks: 1365421
I have to admit I'm not wholly convinced this patch as-is will take care of all the test failures, given that we're increasing the number of items in the navbar and that tends to push things into overflow when not expected, plus there might be more tests that rely on the bookmarks menu button being in the navbar by default, so I pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=272e40c27527 . browser/components/customizableui/ and browser/components/places/tests/browser/ both pass for me with this patch.
Comment on attachment 8894631 [details]
Bug 1383009 - add flexible spacers and library button to the navbar by default,

https://reviewboard.mozilla.org/r/165788/#review170886

::: browser/components/customizableui/CustomizableUI.jsm:407
(Diff revision 1)
> +        // The url bar is now 1 index later, add another one:
> +        let secondSpringIndex = urlbarIndex + 2;

The secondSpringIndex should only be incremented by two if we actually splice in a new spring. This currently always increments by 2.

::: browser/components/customizableui/CustomizableUI.jsm:419
(Diff revision 1)
> +      if (placements.includes("bookmarks-menu-button")) {
> +        let bmbIndex = placements.indexOf("bookmarks-menu-button");

Instead of doing this twice, we should just get the index and compare it with -1.
Attachment #8894631 - Flags: review?(jaws) → review+
Note that these test fixes fix everything except browser/base/content/test/performance/browser_urlbar_search_reflows.js, for which I need UX confirmation about what to do:

- align items in the autocomplete popup with the url bar, even with the spacer and other icons to the left of the URL bar, so creating a large amount of whitespace at the start of the autocomplete popup ( http://imgur.com/a/JfSOH )
- start-align the items so there isn't such a huge amount of whitespace, but then the items don't line up with the url bar anymore.

In the latter case, we'll have to come up with a way of making the reflow test pass.
Flags: needinfo?(shorlander)
Flags: needinfo?(bbell)
shorlander said on slack we could just use 30% of the window width as the cut-off point for where we start ignoring the URL bar position and always position results at the start (left in ltr, right in rtl) edge of the window. I also added a check for a hardcoded 250px (ie, if the distance to the edge is less than that, we align with the url bar) so that we continue to align things in narrow windows. Stephen, can you confirm that works for you?
Flags: needinfo?(bbell)
Comment on attachment 8895056 [details]
Bug 1383009 - fix logic for url bar autocomplete item positioning to allow it to position items in more cases,

https://reviewboard.mozilla.org/r/166184/#review171504

::: browser/base/content/urlbarBindings.xml:1878
(Diff revision 1)
> -          // at most two toolbar buttons between the window edge and the urlbar,
> -          // then consider that as "not too far."  The forward button's
> -          // visibility may have changed since the last time the popup was
> -          // opened, so this needs to happen now.  Do it *before* the popup
> -          // opens because otherwise the items will visibly shift.
> -          let nodes = [...document.getElementById("nav-bar-customization-target").childNodes];
> +          // "too far" as "more than 30% of the window's width AND more than
> +          // 250px"
> +          let boundToCheck = popupDirection == "rtl" ? "right" : "left";
> +          let inputRect = this.DOMWindowUtils.getBoundsWithoutFlushing(aInput);
> +          let startOffset = Math.abs(inputRect[boundToCheck] - documentRect[boundToCheck]);
> +          let alignSiteIcons = startOffset / width <= 0.3 || startOffset < 250;

Precisely the second term should use <=, not <, right?  To match what the comment says.
Attachment #8895056 - Flags: review?(adw) → review+
Got confirmation from Stephen this works for him, so clearing ni.
Flags: needinfo?(shorlander)
Comment on attachment 8894879 [details]
Bug 1383009 - fix tests, BrowserUITelemetry and pocket to deal with the changes to the navbar,

https://reviewboard.mozilla.org/r/166036/#review171966

::: browser/components/uitour/test/browser_UITour_availableTargets.js
(Diff revision 2)
>    return [
>      "accountStatus",
>      "addons",
>      "appMenu",
>      "backForward",
> -    "bookmarks",

Why does adding the flexible spacers cause us to remove the "bookmarks" item. That seems related to some other work.

::: browser/extensions/pocket/bootstrap.js:410
(Diff revision 2)
>      for (let window of browserWindows()) {
>        for (let id of ["panelMenu_pocket", "menu_pocket", "BMB_pocket",
>                        "panelMenu_pocketSeparator", "menu_pocketSeparator",
>                        "BMB_pocketSeparator", "appMenu-library-pocket-button"]) {
> -        let element = window.document.getElementById(id);
> +        let element = window.document.getElementById(id) ||
> +                      window.gNavToolbox.palette.querySelector("#" + id);

Similar question, why does adding flexible spacers cause these to end up in the palette?

I understand if they were in the overflow menu, but that doesn't put them in the palette.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #17)
> Comment on attachment 8894879 [details]
> Bug 1383009 - fix tests, BrowserUITelemetry and pocket to deal with the
> changes to the navbar,
> 
> https://reviewboard.mozilla.org/r/166036/#review171966
> 
> ::: browser/components/uitour/test/browser_UITour_availableTargets.js
> (Diff revision 2)
> >    return [
> >      "accountStatus",
> >      "addons",
> >      "appMenu",
> >      "backForward",
> > -    "bookmarks",
> 
> Why does adding the flexible spacers cause us to remove the "bookmarks"
> item. That seems related to some other work.

I ended up also replacing the bookmarks menu button with the library button in this bug (originally bug 1365421). You reviewed that in the first patch, but it was a while back. :-)

I forgot to update the summary here, and I made the other bug dep on this one, but it's probably better to just dupe it over, so I'll do that in a second.

Note that the reason I did this in 1 patch is because it's basically manipulating the exact same set of defaults, and requires similar migration code, so I anticipated that some tests and potentially other places would otherwise just need updating twice, potentially making things more difficult by having to cope with an intermediate state. So it seemed simplest to just do it in one bug. Whether that was the right decision in hindsight... maybe not. But here we are. :-)

> ::: browser/extensions/pocket/bootstrap.js:410
> (Diff revision 2)
> >      for (let window of browserWindows()) {
> >        for (let id of ["panelMenu_pocket", "menu_pocket", "BMB_pocket",
> >                        "panelMenu_pocketSeparator", "menu_pocketSeparator",
> >                        "BMB_pocketSeparator", "appMenu-library-pocket-button"]) {
> > -        let element = window.document.getElementById(id);
> > +        let element = window.document.getElementById(id) ||
> > +                      window.gNavToolbox.palette.querySelector("#" + id);
> 
> Similar question, why does adding flexible spacers cause these to end up in
> the palette?
> 
> I understand if they were in the overflow menu, but that doesn't put them in
> the palette.

The bookmarks menu button is now in the palette by default. Rather than trying to track when it's inserted/removed and updating it then, I thought it would be simplest to just cope with it being in the palette and adding/removing items to it there.
Summary: Add flexible spaces around the URL and search bar by default → Add flexible spaces around the URL and search bar by default and replace the bookmarks button with the library one
No longer blocks: 1365421
Comment on attachment 8894879 [details]
Bug 1383009 - fix tests, BrowserUITelemetry and pocket to deal with the changes to the navbar,

https://reviewboard.mozilla.org/r/166036/#review172320
Attachment #8894879 - Flags: review?(jaws) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 0a038a63bd9a -d 387b09897512: rebasing 412704:0a038a63bd9a "Bug 1383009 - add flexible spacers and library button to the navbar by default, r=jaws"
merging browser/base/content/browser.xul
merging browser/components/customizableui/CustomizableUI.jsm
merging browser/components/customizableui/test/browser_984455_bookmarks_items_reparenting.js
merging browser/components/places/tests/browser/browser_toolbarbutton_menu_context.js
warning: conflicts while merging browser/base/content/browser.xul! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b9516e57add6
add flexible spacers and library button to the navbar by default, r=jaws
https://hg.mozilla.org/integration/autoland/rev/23ddae2ce26d
fix tests, BrowserUITelemetry and pocket to deal with the changes to the navbar, r=jaws
https://hg.mozilla.org/integration/autoland/rev/4f5ee0246c86
fix logic for url bar autocomplete item positioning to allow it to position items in more cases, r=adw
Depends on: 1389484
Depends on: 1389505
Depends on: 1389573
Verified on Windows, Ubuntu, and Mac.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
Flags: qe-verify+
Depends on: 1389658
Depends on: 1407965
No longer depends on: 1407965
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: