Closed Bug 1317882 Opened 8 years ago Closed 8 years ago

AutoComplete dropdown with Insecure Warning is not extended the correct height at first time opening

Categories

(Toolkit :: Password Manager, enhancement, P1)

Product:
Toolkit
Component:
Password Manager
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox51 --- unaffected
firefox52 + verified
firefox53 --- verified

People

(Reporter: selee, Assigned: timdream)

References

(Blocks 1 open bug)

Details

(Whiteboard: [passwords:fill-UI])

Attachments

(3 files, 3 obsolete files)

Scrollbar at right side of dropdown
76.69 KB, image/png
Details
Warning text not wrapped
458.86 KB, image/png
Details
Bug 1317882 - Calculate overflow for the insecure field richlistitem in the constructor
58 bytes, text/x-review-board-request
MattN
: review+
jcristau
: approval-mozilla-aurora+
Details 
When the dropdown height is not extended to the fit height, there is a scroll-bar at the right side of dropdown.
This incorrect behavior only happens at the first time opening in an autocomplete dropdown with insecure warning. Besides, this can't always be reproduced.
Hi Mike, MattN,

When the incorrect height happens at first time opening, `lastRowRect.bottom`[1] is shorter than the correct one. After that, `lastRowRect.bottom` is given a correct value to calculate the correct height. 

Do you have any suggestions?

[1] http://searchfox.org/mozilla-central/rev/8562d3859b89ac89f46690b9ed2c473e0728d6c0/toolkit/content/widgets/autocomplete.xml#1255
Flags: needinfo?(mconley)
Flags: needinfo?(MattN+bmo)
BTW, I ever tried to wrap `adjustHeight` function in `window.requestAnimationFrame`, but it doesn't work.
When I add some console.log for debug purpose, the reproduce rate can be reduced. I suppose this is relative to the time issue of getBoundingClientRect. However, I am figuring out the root cause.

[1] http://searchfox.org/mozilla-central/rev/8562d3859b89ac89f46690b9ed2c473e0728d6c0/toolkit/content/widgets/autocomplete.xml#1253
Hey - I don't think I fully understand the issue. Do you have manual steps to reproduce so that I can experience the bug?
Flags: needinfo?(mconley) → needinfo?(selee)
STR:
0. Ensure there are some logins for the target page which is insecure.
1. Browse the target page.
2. Trigger the logins autocomplete on username or password field.
3. You will see a drop-down with insecure warning and login items.
4. The drop-down should NOT show a scroll bar except it reaches the bottom of screen.

-----------
In the screen recording [1], the first drop-down shown at 00:10 is with a scroll bar, but the second one at 00:22 is without the bar.

The second one is an expected result, and we need to fix the first case.

[1] https://youtu.be/x8DXM97MC68
Flags: needinfo?(selee) → needinfo?(mconley)
[Tracking Requested - why for this release]:

We need to fix and uplift this one for 52, in order to release the Insecure Password Contextual UI in that release.
tracking-firefox52: --- → ?
Hrm. timedream's theory certainly sounds plausible, but I'm unable to reproduce this locally. Here's what I'm doing: http://www.screencast.com/t/CudtAGGq

Am I accidentally skipping a step?
Flags: needinfo?(mconley) → needinfo?(selee)
Hi Mike, I can reproduce this issue when the window is opened at the first time. (but also the drop-down at first time.)

If this issue can't be reproduced at the first drop-down opened, you need to reopen the window and browse the target page again.
Flags: needinfo?(selee) → needinfo?(mconley)
List the investigation I did today:
A. From the console.log, the construction of #autocomplete-richlistitem-insecure-field happens before adjustHeight(). We might say that DOM construction in _appendCurrentResult() is ready then adjustHeight executed, but there is a setTimeout in _appendCurrentResult() to append the following items.

B. I tried to use getComputedStyle, offsetHeight (get undefined, I suspect it’s for HTMLElement only), and getBoundingClientRect to force reflow, but it doesn’t work.

C. There are 4 relative timers in _invalidate method ():
  _adjustHeightTimeout
  _shrinkTimeout
  _adjustHeightOnPopupShown
  _appendResultTimeout
I suspect this is where race condition happens.

I would like to trace these timer's behaviors if it's worth or no other suggestion.

[1] http://searchfox.org/mozilla-central/rev/62db1c9021cfbde9fa5e6e9601de16c21f4c7ce4/toolkit/content/widgets/autocomplete.xml#1165-1187
A workaround might be to have the insecure warning binding call the adjustHeight method on the popup after it has constructed.
Flags: needinfo?(mconley)
Hum, comment 7 is false (as proven in comment 12). We would need another workaround.
_adjustHeightTimeout Should be the offending timer. Comparing the two stacks (one printed from the constructor and one printed from the adjustHeight), I can see the call branches at _invalidate(), with adjustHeight() called inside a timeout at [1] and autocomplete-richlistitem-insecure-field_XBL_Constructor being triggered from [2] and eventually [3].

We would just need to know what has too happen for layout to grow the item to the correct height, before the setTimeout here, and how to trigger that synchronously, if it has not happened yet.

(And no, getBoundingClientRect() does not cause the height to be corrected at any of these places)

[1] http://searchfox.org/mozilla-central/rev/935627b015aaabbd2dffa90cce051521f22dd7e6/toolkit/content/widgets/autocomplete.xml#1178
[2] http://searchfox.org/mozilla-central/rev/935627b015aaabbd2dffa90cce051521f22dd7e6/toolkit/content/widgets/autocomplete.xml#1187
[3] http://searchfox.org/mozilla-central/rev/935627b015aaabbd2dffa90cce051521f22dd7e6/toolkit/content/widgets/autocomplete.xml#1366
I wonder if this has to do with the XBL binding not being attached until the first time its visible?
(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #16)
> I wonder if this has to do with the XBL binding not being attached until the
> first time its visible?

See http://searchfox.org/mozilla-central/rev/935627b015aaabbd2dffa90cce051521f22dd7e6/toolkit/content/widgets/autocomplete.xml#1366,1369
(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #16)
> I wonder if this has to do with the XBL binding not being attached until the
> first time its visible?

I am a bit confused about the definition of "attached". I can see the constructor is always called at the line 1336 (observed by two inserting two dump()s before and after that line, and one at the constructor), does that means the binding is "attached"?
I'm having a hard time reproducing this in a new profile but see it regularly in my primary profile. Attached is one screenshot in a new profile wherein the text isn't wrapping for one frame in my video recording. I suspect that's the same issue (or related) as I can imagine that Sean's case occurs because the height of all of the rows is calculated in order to know how large to make the popup while the text isn't wrapping. Then the text starts to wrap, making the total height of the rows greater, thus not fitting in the original popup height.

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #18)
> (In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from
> comment #16)
> > I wonder if this has to do with the XBL binding not being attached until the
> > first time its visible?
> 
> I am a bit confused about the definition of "attached". I can see the
> constructor is always called at the line 1336 (observed by two inserting two
> dump()s before and after that line, and one at the constructor), does that
> means the binding is "attached"?

So you see it called synchronously during appendChild?
Flags: needinfo?(MattN+bmo)
For someone who can reproduce this easily, does setting these two properties in a @style attribute before 1336 fix the problem: https://hg.mozilla.org/mozilla-central/rev/3e315a1721ee#l2.23 ?
(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #19)
> So you see it called synchronously during appendChild?

Yes.
There are three solutions I tried:
1. Add inline style="text-overflow: initial; white-space: initial;" in [1] (.ac-text-overflow-container > .ac-tags-text). The height issue is not resolved.
2. Set style attribute `item.setAttribute("style", "text-overflow: initial; white-space: initial;");` in [2]. The height issue is not resolved.
3. Remove `onoverflow="this._onOverflow();"` at [3]. After removing that, the scroll bar is gone, but there is no wrapping now.

Please note these solutions are applied respectively.

[1] http://searchfox.org/mozilla-central/rev/935627b015aaabbd2dffa90cce051521f22dd7e6/toolkit/content/widgets/autocomplete.xml#1474
[2] http://searchfox.org/mozilla-central/rev/935627b015aaabbd2dffa90cce051521f22dd7e6/toolkit/content/widgets/autocomplete.xml#1365
[3] http://searchfox.org/mozilla-central/rev/935627b015aaabbd2dffa90cce051521f22dd7e6/toolkit/content/widgets/autocomplete.xml#1461
The reproducing rate is higher in non-e10s mode.
Hi all,

The patch in attachment 8813514 [details] is a workaround solution I found so far, and I suppose we still need to discuss if `rows[i]._handleOverflow();` is in a proper place or other concerns.
However, I've verified this patch in both e10s and non-e10s mode in 5 times respectively.

The idea is to handle overflow for each item before calculating the drop-down height. This can ensure the overflow can be consider when calculating the height.

Please help to verify this patch for someone who can reproduce this easily. Thank you.
Comment on attachment 8813514 [details]
Bug 1317882 - Fix the height fitting issue of username dropdown in insecure warning case.

Hi MattN, could you help to give this patch a feedback? thanks.
Attachment #8813514 - Flags: feedback?(MattN+bmo)
Sean, thank you very much for investigating. I got an alternative proposal which is more localized; I did not test it many times but I think this works the same way as your patch.


diff --git a/toolkit/content/widgets/autocomplete.xml b/toolkit/content/widgets/autocomplete.xml
index f1a0e8c..92feceb 100644
--- a/toolkit/content/widgets/autocomplete.xml
+++ b/toolkit/content/widgets/autocomplete.xml
@@ -1511,16 +1511,20 @@ extends="chrome://global/content/bindings/popup.xml#popup">
         </xul:description>
       </xul:hbox>
     </content>
     <implementation>
       <constructor><![CDATA[
         let learnMoreLink = document.getElementById("learnMoreLink");
         learnMoreLink.setAttribute("value", this._stringBundle.GetStringFromName("insecureFieldWarningLearnMore"));
         learnMoreLink.setAttribute("href", Services.urlFormatter.formatURLPref("app.support.baseURL") + "insecure-form-field-warning");
+
+        // Unlike other autocomplete items, the height of the insecure warning expends when overflow.
+        // We had calculate the overflow at the of the construction here.
+        this._handleOverflow();
       ]]></constructor>
 
       <property name="_stringBundle">
         <getter><![CDATA[
           if (!this.__stringBundle) {
             this.__stringBundle = Services.strings.createBundle("chrome://passwordmgr/locale/passwordmgr.properties");
           }
           return this.__stringBundle;
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #27)
> +        // Unlike other autocomplete items, the height of the insecure
> warning expends when overflow.

s/expends/expands/, ouch.
Hey Tim,

Thanks a lot for providing your patch and sharing these ideas.

How do you think handling overflow in adjustHeight is more explicit to understand the overflow handling must be done before calculating the height?

If there is any timing issue happens like this (e.g. changing overflow or DOM element after constructor.) in the future, this solution can handle these cases.
(In reply to Sean Lee [:seanlee][:weilonge] from comment #29)
> Hey Tim,
> 
> Thanks a lot for providing your patch and sharing these ideas.
> 
> How do you think handling overflow in adjustHeight is more explicit to
> understand the overflow handling must be done before calculating the height?
> 
> If there is any timing issue happens like this (e.g. changing overflow or
> DOM element after constructor.) in the future, this solution can handle
> these cases.

I don't understand _handleOverflow() enough to have an option on this. The comment said the method is used to truncate the displayed string, but we wrap instead of truncate for the insecure warning item, so I don't really know why we need to have the method called for the height to properly expand.

What I do know is, we probably don't need to call it on other items when adjusting the height, and doing that in your patch is probably necessary CPU cycles and CO2 emissions.
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #31)
> I am investigating whether or not we could layout the insecure warning item
> without JavaScript triggered by the overflow event. Our layout is too simple
> to have a need on that.

Never mind, it's probably more complex than I thought. Ensure _handleOverflow() is called (and the max-width prop is calculated and set so the text wraps) should be the right idea here.
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #30)
> doing that in your patch is probably necessary CPU
> cycles and CO2 emissions.

* UNnecessary CPU cycles ...
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #27)
> Sean, thank you very much for investigating. I got an alternative proposal
> which is more localized; I did not test it many times but I think this works
> the same way as your patch.

Hey Tim,

When I try your approach I get:
> JavaScript error: chrome://global/content/bindings/autocomplete.xml, line 1093: TypeError: this.richlistbox.ensureElementIsVisible is not a function

Is there more to this approach? I agree it's more localized and would like to consider it.
Flags: needinfo?(timdream)
Comment on attachment 8813514 [details]
Bug 1317882 - Fix the height fitting issue of username dropdown in insecure warning case.

https://reviewboard.mozilla.org/r/94946/#review96224

::: toolkit/content/widgets/autocomplete.xml:1247
(Diff revision 1)
>                let lastVisibleRowRect = rows[this.maxRows - 1].getBoundingClientRect();
>                let visibleHeight = lastVisibleRowRect.bottom - firstRowRect.top;

Don't these two lines depend on the correct height already? I think the addition would have to be above these for the case where `numRows > this.maxRows`.
Sorry, nevermind that, it works when I make the change correctly.
Flags: needinfo?(timdream)
Attachment #8812677 - Attachment is obsolete: true
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #32)
> (In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment
> #31)
> > I am investigating whether or not we could layout the insecure warning item
> > without JavaScript triggered by the overflow event. Our layout is too simple
> > to have a need on that.
> 
> Never mind, it's probably more complex than I thought. Ensure
> _handleOverflow() is called (and the max-width prop is calculated and set so
> the text wraps) should be the right idea here.

Yeah, the relevant line is:
> this._titleText.style.maxWidth = titleMaxWidth + "px";

adw/mak, do you have any preference for the two approaches? My concern with the more general approach is that it may be causing reflows as-written. Do we have a good way to benckmark a patch like this (e.g. Talos) or do we just look at aggregate data from awesomebar telemetry normally?

I'm leaning towards taking the localized approach for Aurora and having a follow-up to investigate the more general approach. Perhaps you know of another solution to this problem instead?
Flags: needinfo?(mak77)
Flags: needinfo?(adw)
Just a remind here that the constructor of <binding id="autocomplete-richlistitem-insecure-field"> has been removed in bug 1318537 for central. Aurora doesn't have the changes yet.
Drew worked more extensively on the overlow problem, so I'm leaving the final answer to him.

We don't have a good benchmark for reflows here, and indeed I suspect we're already doing a lot of them. We have some telemetry, but in the locationbar case the time needed to rountrip I/O is larger than any paint time.
I'd also vote for the local approach, for a couple reasons: first it's not going to affect other consumers, second it's not that we add stuff here every other day so doesn't look like atm there's the need for a broader fix.
Flags: needinfo?(mak77)
Sean or Tim - should this bug be assigned to one of you?  Or do we need to find someone to take it?  Thanks!
Sean is on PTO so I'm making sure one of the two approaches lands. I'll re-assign to the person who's fix we use.
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
I agree with Marco:

(In reply to Marco Bonardo [::mak] from comment #41)
> I'd also vote for the local approach, for a couple reasons: first it's not
> going to affect other consumers, second it's not that we add stuff here
> every other day so doesn't look like atm there's the need for a broader fix.

If we end up adding more wrappable things, it would be worth coming up with a more general and performant approach (which may be Sean's approach), but right now I think it's wiser to take the more narrow approach by special-casing this case.
Flags: needinfo?(adw)
I can submit a patch today.
Assignee: MattN+bmo → timdream
Comment on attachment 8815097 [details]
Bug 1317882 - Calculate overflow for the insecure field richlistitem in the constructor

OK, we'll land this approach then. Thanks Drew and Marco for the feedback. I didn't want it to seem like we were avoiding the general solution but I also preferred the more localized approach.
Attachment #8815097 - Flags: review+
Attachment #8813514 - Attachment is obsolete: true
Attachment #8813514 - Flags: feedback?(MattN+bmo)
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8617a600ec4
Calculate overflow for the insecure field richlistitem in the constructor. r=MattN
Comment on attachment 8815596 [details]
Bug 1317882 - Make sure contextual warning text wraps before we probe for it's height,

Cancelling my push -- this is for testing some reviewboard quirks only.
Attachment #8815596 - Attachment is obsolete: true
Attachment #8815596 - Flags: review?(MattN+bmo)
https://hg.mozilla.org/mozilla-central/rev/a8617a600ec4
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment on attachment 8815097 [details]
Bug 1317882 - Calculate overflow for the insecure field richlistitem in the constructor

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1289913
[User impact if declined]: Incorrect height on the autocomplete menu when the menu is first opened.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: Yes, verified manually.
[Needs manual test from QE? If yes, steps to reproduce]: See comment 0. Manually verified already.
[List of other uplifts needed for the feature/fix]: See dependencies.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Patch only affect autocomplete insecure item locally.
[String changes made/needed]: None.
Attachment #8815097 - Flags: approval-mozilla-aurora?
Comment on attachment 8815097 [details]
Bug 1317882 - Calculate overflow for the insecure field richlistitem in the constructor

fix a bug in a new feature in aurora52
Attachment #8815097 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Needs rebasing around bug 1318537 for the Aurora uplift.
Flags: needinfo?(timdream)
Blocks: 1289913
No longer depends on: 1289913
Flags: needinfo?(timdream)
Depends on: 1325695
I want to verify this bug but I'm not able to reproduce it.
I'm not able to reproduce it either on either Ubuntu 16.04, Windows 10 and Mac OSX 10.10 with today's 52.0a2 or 53.0a1 build: 20170122004006.
Based on above setting the fix as verified.
Status: RESOLVED → VERIFIED
status-firefox52: fixed → verified
status-firefox53: fixed → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: