Closed Bug 1402272 Opened 4 years ago Closed 4 years ago

Evaluate a different approach for adjustSiteIconStart

Categories

(Firefox :: Address Bar, defect, P1)

Product:
Firefox
Component:
Address Bar
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox56 --- wontfix
firefox57 + fixed
firefox58 --- fixed

People

(Reporter: mak, Assigned: adw)

References

Details

(Keywords: perf, Whiteboard: [fxsearch])

Attachments

(5 files)

searchbox.png
648.17 KB, image/png
Details
Bug 1402272 - Replace adjustSiteIconStart with padding on all richlistitems.
59 bytes, text/x-review-board-request
mak
: review+
ritu
: approval-mozilla-beta+
Details
sshot.png
136.13 KB, image/png
Details
sshot2
88.74 KB, image/png
Details
search-dropdown.jpg
963.34 KB, image/jpeg
Details 
I was discussing with Florian about an alternative possible approach for adjustsiteIconStart.

Instead of adjusting the starting point of each richlistitem, we'd rather try to add a margin (or a spacer) on the richlistbox.
That margin should change:
1. then customization changes
2. when the window is resized

The margin should also take into account the star/switchtotab icon on the left that should be hidden but take space yet.

There would be one visual change related to this, the hover/selection wouldn't go through the whole panel, but it would instead be restricted to the RLB width.
Keywords: perf
(In reply to Marco Bonardo [::mak] from comment #0)

> There would be one visual change related to this, the hover/selection
> wouldn't go through the whole panel, but it would instead be restricted to
> the RLB width.

If this visual change isn't desirable/acceptable, an alternative solution could be to set a CSS rule on the document that would apply to all awesomebar items at once.
Philipp, do you think we could experiment with selection/hover having a smaller width than the whole awesomebar popup? I t would more or less look like the Chrome one (module the radius that is TBD)
Flags: needinfo?(philipp)
Blocks: 1403071
Blocks: 1381427
(In reply to Marco Bonardo [::mak] from comment #2)
> Philipp, do you think we could experiment with selection/hover having a
> smaller width than the whole awesomebar popup? I t would more or less look
> like the Chrome one (module the radius that is TBD)

Redirecting as phlsa is on PTO.
Flags: needinfo?(philipp) → needinfo?(shorlander)
(In reply to Panos Astithas [:past] (56 Regression Engineering Owner) (please ni?) from comment #3)
> (In reply to Marco Bonardo [::mak] from comment #2)
> > Philipp, do you think we could experiment with selection/hover having a
> > smaller width than the whole awesomebar popup? I t would more or less look
> > like the Chrome one (module the radius that is TBD)
> 
> Redirecting as phlsa is on PTO.

I just remembered that Eric has thought about the result panel a lot, too.
Flags: needinfo?(epang)
Assignee: nobody → adw
Status: NEW → ASSIGNED
Paolo, Marco said you're already working on this, is that right?  I didn't mean to steal it from you.
Flags: needinfo?(paolo.mozmail)
Attached image searchbox.png — 
Sorry for the rough mock up, but would we be able to do something like this?  Have the search drop down centered under the location bar with some extra padding on the right and left edges to account for the star/switch to tab icons and maintaining balance on the right side?

If possible we'll also have to determine a min-width for smaller windows where the location bar becomes too small.
Flags: needinfo?(shorlander)
Flags: needinfo?(epang)
This implements Eric's mockup in comment 6: the popup is slightly wider than the textbox and centered on the textbox.  It allows us to get rid of a fair amount of code.  I'll push this to try and get some try builds for people to test.
Flags: needinfo?(paolo.mozmail)
Fixes for performance reflow tests.  Looks like we can remove even more expected reflows than was obvious.
(In reply to Drew Willcoxon :adw from comment #5)
> Paolo, Marco said you're already working on this, is that right?  I didn't
> mean to steal it from you.

Don't worry, that's great, go on! Mostly I just removed exactly the same code, although I was adding the margin to the inside, because I started before the most recent mockup.

Something you may want to do is to give a minimum width to the popup, so it can expand when the location bar is very small - the idea with the original was that the results would be as large as the window when a certain threshold was reached. This threshold could even be an about:config setting and it could be set to a high value to always use the entire space (see bug 1381427 comment 5). Marco, what do you think?
Flags: needinfo?(mak77)
Duplicate of this bug: 1381427
We surely have a problem with small windows, we always had to be honest, so it's not a strict blocker, the full width made it just a little bit better. I agree we may want to do something (simple) about it, and a min-width looks like simple enough for an MVP. Since we would like this in 57, I'd not go much further. I don't think it should be exposed through a pref, we can surely find a decent compromise.
Flags: needinfo?(mak77)
Note that we should get ux-review regardless.
Attached image sshot.png — 
I tested the Try build, it works surprisingly well, I can notice a clear painting speed difference!

On the visual side, the disconnection from the input field looks a bit strange to me. Obviously it's matter of personal taste, I just find a bit odd that the panel is "slightly" larger than the input field. I wouldn't care if the panel would be window-wide and the contents limited, since it would be less noticeable.

I think the best thing is having Eric actually test the try build and evaluate the final result.

Here I'm attaching a screenshot comparison of what this Try looks like VS what Chrome looks like on Windows, that was my initial suggestion (keep the panel window-wide, but limit contents). Looks like Chrome doesn't act like this on all the platforms, so I'm adding it for completion.
Flags: needinfo?(epang)
Attached image sshot2 — 
and this is more or less what I had in mind.
Again, this is just for completion, I don't really want to do UX :)
wow, that looks really… broken (both screenshots!). Is there a need to do this? What's wrong with the full width design? Also note that, for me, Chrome does exactly the same as Firefox now and not the same as in your screenshot.
Attached image search-dropdown.jpg — 
Hi Drew, thanks for your quick work on this, just tested the build. Can you make a few updates?

1. Reduce the search box by 21px on both sides.  This centers the star/switch to tab spacing.

2. Move the search box by 1 px so it's aligned with the line of the chrome.

The reason I'm moving away from the full width design is because of the white space that is created on both sides which is even further highlighted now with the shortened hover state.

Please flag me for review when the changes are in :)
Flags: needinfo?(epang) → needinfo?(adw)
Eric, let me make sure I understand what you mean by "search box."  Do you mean the popup/panel/listbox underneath the textbox that the bookmarks, history, search suggestions are shown in?  I think so but want to make sure.
Flags: needinfo?(adw) → needinfo?(epang)
Re: small windows: That doesn't actually seem to be a problem, at least when I test on macOS.  (I should try on Windows.)  The window itself doesn't shrink past ~330px, and the urlbar doesn't shrink past ~200px.  So ~200px (plus padding on the sides) is the natural min width of the popup.  TBH it looks weird even then, without making the popup wider, because it extends past the bounds of the window. :-/  Giving it a min width beyond that would make it look even weirder IMO.
For comparison I tested with Safari, which uses a similar style: a popup slightly wider than and centered on the textbox.  It's not possible to shrink the window at all beyond ~500px, and the urlbar remains centered and completely visible with room to spare on both sides of it.  So the popup still looks fine.  In contrast, when I shrink Firefox as small as possible, the urlbar is cut off on the right side.

IMO instead of giving the popup a min width, we should do a better job at shrinking the window and giving it or the urlbar a min width, with enough space on its sides so that the popup looks OK.
This addresses Eric's comment 21.  I'll post try builds when they're ready.  I'm still trying to figure out the best way to stop the urlbar from being cropped in narrow windows, and maybe that's best for a follow-up.
FWIW, I am concerned about the amount of risk this entails for 57. Do we really need to revise this for 57?
(In reply to Drew Willcoxon :adw from comment #22)
> Eric, let me make sure I understand what you mean by "search box."  Do you
> mean the popup/panel/listbox underneath the textbox that the bookmarks,
> history, search suggestions are shown in?  I think so but want to make sure.

Yep!
Flags: needinfo?(epang)
(In reply to :Gijs from comment #28)
> FWIW, I am concerned about the amount of risk this entails for 57. Do we
> really need to revise this for 57?

To be fair, it's a pretty small amount of risk because all it's really doing is (1) changing some constants in JS and CSS (margins and such) and (2) removing now-unused code.  I don't know how bad the reflows are, but if they're at all non-negligible, the risk is low enough IMO it seems OK.

I'd also add that if we're going to change the width of the popup again, better to do it along with all the other UI changes in Photon.
(In reply to Eric Pang [:epang] UX from comment #21)
> The reason I'm moving away from the full width design is because of the
> white space that is created on both sides which is even further highlighted
> now with the shortened hover state.

There is a lot of white space but a smaller "frame" does not really solve the problem, the space is still unused. Instead it makes that the design looks really broken. Also Chrome does not do this. Chrome does exactly this what Firefox is doing without this patch.

If the white space is a concern just let the content begin on the left and not in the middle. This is the real problem. Then there will also be more space for displaying the page titles and URLs. Title and URL are important information, these deserve space.
I think the new patch looks better and still works pretty well. Regarding the risk/reward, if we exclude the OMGCHANGE visual problem, the rewards are largely overwhelming the small risks. We'll be more specific at time of the uplift request, but in practice this fixes 3 pretty obvious issues at the cost of simpler code.
Blocks: 1350535
Comment on attachment 8913498 [details]
Bug 1402272 - Replace adjustSiteIconStart with padding on all richlistitems.

https://reviewboard.mozilla.org/r/184848/#review190502

I'd suggest you to also run firefox-ui-functional tests on Try since some of them are interacting with the locationbar
Also, If you want to verify the stability (vs intermittent failures) of the reflow tests, you can use:
./mach try -b do -p linux,linux64,macosx64,win32,win64 -u none -t none --rebuild 10 --artifact browser/base/content/test/performance

The patch looks mostly good, I have a few concern to address/answer.

::: browser/base/content/urlbarBindings.xml:1930
(Diff revision 3)
>            // which is an offset from the bottom of the input, subtract the
>            // bottom of the navbar from the buttom of the input.
>            let yOffset = Math.round(
>              this.DOMWindowUtils.getBoundsWithoutFlushing(document.getElementById("nav-bar")).bottom -
>              this.DOMWindowUtils.getBoundsWithoutFlushing(aInput).bottom);
> +          if (AppConstants.platform == "macosx") {

could you please add a comment about the reason for this code snippet? Is it a rounding problem or a deliberate design choice?

::: browser/themes/osx/browser.css:359
(Diff revision 3)
> +  background-position: right 29px center;
> +}
> +
> +#PopupAutoCompleteRichResult > deck[anonid="search-suggestions-notification"] > hbox[anonid="search-suggestions-opt-out"] {
> +  padding-inline-start: 29px;
> +}

why is this the case only on Mac (maybe worth a brief comment)

::: browser/themes/shared/urlbar-autocomplete.inc.css
(Diff revision 3)
>  }
>  
>  .autocomplete-richlistitem {
>    min-height: 30px;
>    font: message-box;
> -  border-radius: 2px;

If I'm not wrong, these changes will affect any autocomplete field, since also web forms now use a richlistbox autocomplete.
The only tree autocomplete left is the search bar.

Are these changes wanted globally?

Off-hand looks like this shared css is not very well named, since all of this is not just about the urlbar. it should probably just be autocomplete.inc.css

If we want these changed to only apply to the urlbar, we need to override those rules with more specific #PopupAutoCompleteRichResult > richlistbox ones

::: browser/themes/shared/urlbarSearchSuggestionsNotification.inc.css:69
(Diff revision 3)
>  }
>  
>  /* Opt-out hint */
>  
>  #PopupAutoCompleteRichResult > deck[anonid="search-suggestions-notification"] > hbox[anonid="search-suggestions-opt-out"] {
> +  padding-inline-start: 28px;

Did you test the opt-out experience on all the 3 tier-1 platforms? I don't care much about the opt-in one, since we're likely to remove that code soon.

How is it working in small windows now? (this is my biggest concern honestly) What's the minimum window width where it stays readable?
Attachment #8913498 - Flags: review?(mak77)
[Sorry Marco, I meant for this new patch to become a new changeset on mozreview, not a new diff based on the old one.  It's completely different.]

Here's a different approach.  It doesn't change the UI at all, just the implementation of how we adjust the items' margin.  It makes these changes:

(1) Instead of visiting each item separately and setting width on a spacer (or margin or padding or whatever), add a CSS rule to .autocomplete-richlistitem:

  padding-inline-start: var(--item-padding-start);

And then set --item-padding-start on the popup when siteIconStart is set.

(2) But when siteIconStart is set and the new value is the same as the old, don't do anything.  That way we avoid visiting each item and handling over/underflow.  (We were already doing this.)

(3) There's no need to call adjustSiteIconStart (or more generally to adjust the site icon start) in _reuseAcItem.

(4) When the popup is opened, set siteIconStart before invalidating the popup and adding any more items to it.  That way we avoid visiting newly added items twice, once when they're added and again when the possibly new siteIconStart is set.

(5) We were calling getBoundingClientRect() on the #identity-icon.  Does that cause a flush?  I changed it to dwu.getBoundsWithoutFlushing() to be safe.

* * *

I started thinking about this earlier today when Eric emailed me and asked why we wanted the UI change.  I started writing out an answer and realized I didn't really understand what we were gaining with it.  We still need to visit each item and handle over/underflow when the popup width changes.

I talked with Stephen today on IRC, plus Eric over email as I mentioned.  If we still want to go with the earlier approach, Stephen requested a couple of tweaks.

This fixes the alignment problem when the urlbar is at the edge of the window too.
Flags: needinfo?(epang)
(In reply to Drew Willcoxon :adw from comment #36)
> I started thinking about this earlier today when Eric emailed me and asked
> why we wanted the UI change.  I started writing out an answer and realized I
> didn't really understand what we were gaining with it.  We still need to
> visit each item and handle over/underflow when the popup width changes.

Indeed we didn't really a UI change, my proposal of changing the selection was because that was a consequence of shrinking the richlistbox, that had its own advantages:
1. no risk of getting the alignment wrong, since the RLB would enforce it
2. it was also fixing bug 1381427 for free, now I assume we'll have to reopen and fix that apart

I'm happy to go with the alternative approach if that means an higher probability of getting an uplift approval, and I assume not changing the UI helps a lot.
(In reply to Marco Bonardo [::mak] from comment #38)
> (In reply to Drew Willcoxon :adw from comment #36)
> > I started thinking about this earlier today when Eric emailed me and asked
> > why we wanted the UI change.  I started writing out an answer and realized I
> > didn't really understand what we were gaining with it.  We still need to
> > visit each item and handle over/underflow when the popup width changes.
> 
> Indeed we didn't really a UI change, my proposal of changing the selection
> was because that was a consequence of shrinking the richlistbox, that had
> its own advantages:
> 1. no risk of getting the alignment wrong, since the RLB would enforce it
> 2. it was also fixing bug 1381427 for free, now I assume we'll have to
> reopen and fix that apart
> 
> I'm happy to go with the alternative approach if that means an higher
> probability of getting an uplift approval, and I assume not changing the UI
> helps a lot.

At this stage I would prefer to not change the UI if it's possible to fix the alignment and perf issues in another way.

There are some design benefits to the non-full width approach that I think we should explore. But we should take the time to fully consider any layout changes.
Comment on attachment 8913498 [details]
Bug 1402272 - Replace adjustSiteIconStart with padding on all richlistitems.

https://reviewboard.mozilla.org/r/184848/#review191014

::: browser/base/content/urlbarBindings.xml:1897
(Diff revision 4)
>              let offset = documentRect.left - elementRect.left;
>              this.style.marginLeft = offset + "px";
>            }
>  
> +          // Adjust the direction of the autocomplete popup list based on the textbox direction, bug 649840
> +          var popupDirection = aElement.ownerGlobal.getComputedStyle(aElement).direction;

I know it's old code, so we don't have to fix it here, but do we actually need to getComputedStyle at every opening, rather than caching it?
It could break RTL testing in the same window if the popup is opened before switching the pref, but I don't think that's a good enough argument.
Likely worth a follow-up bug for 58.

::: toolkit/themes/osx/global/autocomplete.css:101
(Diff revision 4)
>  .ac-type-icon {
>    width: 16px;
>    height: 16px;
>    max-width: 16px;
>    max-height: 16px;
> -  margin-inline-start: 16px;
> +  margin-inline-start: 6px;

out of curiosity, any idea why Mac is the only one differing by 1px?
Attachment #8913498 - Flags: review?(mak77) → review+
I forgot to remove the type-icon-spacer in this new patch.  Will do that.

(In reply to Marco Bonardo [::mak] from comment #40)
> ::: browser/base/content/urlbarBindings.xml:1897
> (Diff revision 4)
> >              let offset = documentRect.left - elementRect.left;
> >              this.style.marginLeft = offset + "px";
> >            }
> >  
> > +          // Adjust the direction of the autocomplete popup list based on the textbox direction, bug 649840
> > +          var popupDirection = aElement.ownerGlobal.getComputedStyle(aElement).direction;
> 
> I know it's old code, so we don't have to fix it here, but do we actually
> need to getComputedStyle at every opening, rather than caching it?

I had the same thought.  We could definitely cache it.  Since we both had that thought, and it's a small change, I'll go ahead and make it now.  I think it should be OK.

> ::: toolkit/themes/osx/global/autocomplete.css:101
> (Diff revision 4)
> >  .ac-type-icon {
> >    width: 16px;
> >    height: 16px;
> >    max-width: 16px;
> >    max-height: 16px;
> > -  margin-inline-start: 16px;
> > +  margin-inline-start: 6px;
> 
> out of curiosity, any idea why Mac is the only one differing by 1px?

I don't know, but I'm really curious, and it bothers me that I couldn't figure it out.  The difference is necessary to pixel-perfectly align the item icons with the site icon on macOS (e.g., the magnifying glass).
(In reply to Drew Willcoxon :adw from comment #41)
> I don't know, but I'm really curious, and it bothers me that I couldn't
> figure it out.  The difference is necessary to pixel-perfectly align the
> item icons with the site icon on macOS (e.g., the magnifying glass).

Hm. right, that's why I wonder if rounding is involved considered that in most cases Mac is the only hi-dpi.
Could it be that we call siteIconStart with a non-rounded value and we should be rounding?
(In reply to Marco Bonardo [::mak] from comment #42)
> Could it be that we call siteIconStart with a non-rounded value and we
> should be rounding?

Ah yeah maybe, I'll see.  (I only saw this comment after I pushed a new mozreview revision just now.)
Yeah, round()ing fixed it.

I'll push this to try one last time before landing.  Other changes:

* Replace other node.getBoundingClientRect() calls with dwu.getBoundsWithoutFlushing(node)

* Cache the popup direction (from getComputedStyle) instead of getting it every time the popup opens.  I took a quick look at caching it *before* the popup is even opened at all, but it's not clear to me when exactly the popup's input is set, and then based on that when would be a good time to cache the direction.  Something for follow-up work.

* Fix an error in the previous patch where I was using `popupDirection` before setting it

* Remove type-icon-spacer

* Remove a couple more expected reflows from the tests that are now no longer expected
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f580475d2739
Replace adjustSiteIconStart with padding on all richlistitems. r=mak
https://hg.mozilla.org/mozilla-central/rev/f580475d2739
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
[Tracking Requested - why for this release]: https://bug1403781.bmoattachments.org/attachment.cgi?id=8913004
status-firefox57: --- → affected
tracking-firefox57: --- → ?
status-firefox56: --- → wontfix
Comment on attachment 8913498 [details]
Bug 1402272 - Replace adjustSiteIconStart with padding on all richlistitems.

Approval Request Comment
[Feature/Bug causing the regression]: The bug is there from some time, but recently due to the removal of various layout flushes it became more evident
[User impact if declined]: Results in the location bar popup can be misaligned, worst case seen so far is https://bug1403781.bmoattachments.org/attachment.cgi?id=8913004 while a more common case is https://bug1403071.bmoattachments.org/attachment.cgi?id=8912112. Additionally (but not the main purpose of the uplift) this is likely to help with large jank some users are seeing when typing in the Address Bar (bug 1402130).
[Is this code covered by automated tests?]: no, it's tricky
[Has the fix been verified in Nightly?]: yes, I just did
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not particularly
[Why is the change risky/not risky?]: we are pretty much setting a css property, the old code was more complex
[String changes made/needed]: none
Attachment #8913498 - Flags: approval-mozilla-beta?
Comment on attachment 8913498 [details]
Bug 1402272 - Replace adjustSiteIconStart with padding on all richlistitems.

This fix could reduce the jank in awesome bar, Beta57+
Attachment #8913498 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Depends on: 1406509
You need to log in before you can comment on or make changes to this bug.