Suggestions twitch when I edit string in location bar

VERIFIED FIXED in Firefox 48

Status

()

defect
P3
normal
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: arni2033, Assigned: adw)

Tracking

(Depends on 2 bugs, {regression})

Trunk
Firefox 49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 verified, firefox49 verified)

Details

(Whiteboard: [fxsearch])

Attachments

(3 attachments)

Reporter

Description

3 years ago
>>>   My Info:   Win7_64, Nightly 48, 32bit, ID 20160409030219
STR:
1. Focus urlbar, keep typing some text (but don't _hold_ any key)

AR:  The first suggestion is twitching enormously
ER:  Should not twitch

Note:  This is especially bad when I edit long urls like "data:text/html,..." testcases.

This is regression from bug 1181078. Regression range:
> https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=c131636dae420773342d1439b6086b6c62b222f4&tochange=061165ac1ff9e076ec51ea268878efa751173511
Priority: -- → P3
Whiteboard: [fxsearch]
Assignee

Comment 1

3 years ago
Thank you for filing this.
Assignee: nobody → adw
Status: NEW → ASSIGNED
Assignee

Comment 2

3 years ago
The problem is that _adjustAcItem removes items' max-widths at the beginning and then handles overflow at the end -- while popup is open.  So the item visually overflows at the beginning and then gets visually truncated at the end.

This patch stops removing max-width when the popup is open.  That means that items can't grow bigger while the popup is open, but I don't think it's actually possible on any platform for the popup to grow wider while it's open, so that shouldn't matter.  Even if that were somehow possible I don't think that would be bad.

While I'm changing truncation, I noticed that action items get truncated too soon.  They end up not wide enough.  It's especially noticable with the heuristic result.  The problem is the Math.max(urlRect.width, actionRect.width) calculation.  The URL rect is hidden with visibility:collapse, so its rect actually still has non-zero width (apparently).  Using display:none instead makes it zero, so the result is better.

But after I changed that, I noticed when pressing the Down arrow key in the urlbar, the heuristic result was incorrectly truncated, as if both the action and url rects had zero width.  And in fact that was what was happening, because when the popup first opens, no result is selected, and then a split second later the heuristic result is selected.

So, this also initializes selectedIndex=0 instead of -1 in urlbar-rich-result-popup's _openAutocompletePopup.  Not only does that fix the problem, it makes the popup look better overall because there's not a split second where nothing is selected.  (And it fixes bug 1266374.)  We can't remove the selectedIndex=0 in onResultsAdded though because the selectedIndex is reset whenever the popup changes.

Review commit: https://reviewboard.mozilla.org/r/48227/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48227/
Attachment #8743995 - Flags: review?(mak77)
Comment on attachment 8743995 [details]
MozReview Request: Bug 1266375 - Don't remove max-width on awesomebar popup items when the popup is open. r?mak

https://reviewboard.mozilla.org/r/48227/#review45133

::: browser/base/content/urlbarBindings.xml:1354
(Diff revision 1)
>  
>            this.mInput = aInput;
> -          this.selectedIndex = -1;
> +
> +          // Initialize the selected index to the heuristic result so that it's
> +          // selected the moment the popup opens.
> +          this.selectedIndex = 0;

Looks like this breaks the default typed-history dropdown when nothing is typed in the location bar

::: toolkit/content/widgets/autocomplete.xml:1837
(Diff revision 1)
>        </method>
>  
>        <method name="_adjustAcItem">
>          <body>
>            <![CDATA[
> +          if (!this.parentNode.parentNode.mPopupOpen) {

just .popupOpen?
Attachment #8743995 - Flags: review?(mak77)
Assignee

Comment 5

3 years ago
What do you mean by broken?  All I see is that the first result is selected in that case when it shouldn't be.  This fixes it:

this.selectedIndex = this._isFirstResultHeuristic ? 0 : -1;

Other than that there are some try failures I need to look at.
Assignee

Comment 6

3 years ago
Comment on attachment 8743995 [details]
MozReview Request: Bug 1266375 - Don't remove max-width on awesomebar popup items when the popup is open. r?mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48227/diff/1-2/
Attachment #8743995 - Flags: review?(mak77)
(In reply to Drew Willcoxon :adw from comment #5)
> What do you mean by broken?  All I see is that the first result is selected
> in that case when it shouldn't be.

yeah sorry, I meant that. bad wording.
Comment on attachment 8743995 [details]
MozReview Request: Bug 1266375 - Don't remove max-width on awesomebar popup items when the popup is open. r?mak

https://reviewboard.mozilla.org/r/48227/#review45803
Attachment #8743995 - Flags: review?(mak77) → review+

Comment 11

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/597390d44c49
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Assignee

Comment 12

3 years ago
Approval Request Comment
[Feature/regressing bug #]: New awesomebar popup design bug 1181078
[User impact if declined]: Bad UX when typing very long strings into the urlbar
[Describe test coverage new/current, TreeHerder]: Covered by existing tests
[Risks and why]: Low risk, small patch, lots of time to ride Aurora and then Beta
[String/UUID change made/needed]: None
Attachment #8746323 - Flags: approval-mozilla-aurora?
Blocks: 1266374
Hello Arni2033, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(arni2033)
Comment on attachment 8746323 [details] [diff] [review]
Aurora 48 patch

Improving awesome bar UX when typing long strings, Aurora48+
Attachment #8746323 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Reporter

Comment 16

3 years ago
>>>   My Info:   Win7_64, Nightly 49, 32bit, ID 20160504043118
It is still possible for label "Search with [default search]" to promptly twitch to the right and left
(twice per keypress) when the first suggestion overflows. There's no other issues.
I don't like that twitching too, because in my view nothing should twitch twice per 1 user action and because "Search with" label promptly appears 5px to the right from the rightmost allowed position.
It's a question for Marco, whether this is expected behavior.

First part of the video shows described twitching. Second part shows that label "Search with..." isn't affected by changing lenthg of suggestion if ellipsis is shown.
> screencast:   https://dl.dropboxusercontent.com/s/9h0twmkrwen7sug/bug%201266375%20comment%2016.webm
Flags: needinfo?(arni2033) → needinfo?(mak77)
(In reply to arni2033 from comment #16)
> >>>   My Info:   Win7_64, Nightly 49, 32bit, ID 20160504043118
> It is still possible for label "Search with [default search]" to promptly
> twitch to the right and left
> (twice per keypress) when the first suggestion overflows. There's no other
> issues.
> I don't like that twitching too, because in my view nothing should twitch
> twice per 1 user action and because "Search with" label promptly appears 5px
> to the right from the rightmost allowed position.
> It's a question for Marco, whether this is expected behavior.

Clearly the fact the text flickers is never expected.
On the other side, the likely for the user to type a so long text to cause that issue is minimal, and the effect is not so annoying to cause big troubles.
While it's a valid bug, I don't think we could spend time on it, cause the fix would be more expensive than the benefit, so it's more likely to stay unfixed (available for the community to fix) if filed.
Flags: needinfo?(mak77)
Reporter

Updated

3 years ago
Depends on: 1271094
Verified issue fixed on Windows 10 x64 and Ubuntu 12.04 x86 using:
- Aurora 48.0a2, build ID 20160508004021
- Nightly 49.0a1, build ID 20160508030214.
Status: RESOLVED → VERIFIED
Reporter

Updated

3 years ago
Depends on: 1278549
Version: unspecified → Trunk
Reporter

Updated

3 years ago
Depends on: 1240217
You need to log in before you can comment on or make changes to this bug.