[Tracking Requested - why for this release]: new feature It seems like the max-width used for wrapping in the insecure field warning isn't recalculated when the focus is moved to a new field (having a different width). This can look very bad and lead to clipping of text in the warning or in the opposite direction leads to useless ugly space below or to the right of the warning. Bug 1317882 made this recalculate in the XBL constructor but perhaps that constructor isn't called when moving between fields. Tim/Sean, since you both investigated bug 1317882, do you have ideas to fix this or would you be able to help out for 52? Thanks. Perhaps the other approach in that bug would have fixed this?

STR: Focus a login field of one width then focus a login field of a different width.

I take this first since I have some bandwidth now.

There are two issues in these screenshot: 1. attachment 8821625 [details] for size=20 to 30 - the weird white space at bottom of warning. 2. attachment 8821626 [details] for size=30 to 20 - the scrollbar is still there even bug 1317882 is resolved. I found the issue in radiotunes.com (attachment 8811131 [details]) is still there. The solutions for these two issues are similar probably. I am going to check these at the same time.

Comment on attachment 8821670 [details] Bug 1325695 - Handle Overflow and OverUnderflow cases when adjusting the height of autocomplete popup.; Hi Mattn, could you help to verify this WIP patch? the solution's idea is below. (In reply to Sean Lee [:seanlee][:weilonge] from comment #5) > There are two issues in these screenshot: > 1. attachment 8821625 [details] for size=20 to 30 - the weird white space at > bottom of warning. This is an OverUnderflow case, so it can be resolved with `rows[i].handleOverUnderflow();`. > 2. attachment 8821626 [details] for size=30 to 20 - the scrollbar is still > there even bug 1317882 is resolved. I found the issue in radiotunes.com > (attachment 8811131 [details]) is still there. This is a Overflow case, so it can be resolved with `rows[i]._handleOverflow();`. > > The solutions for these two issues are similar probably. I am going to check > these at the same time. I am going to investigate if there is any better place or logic to handle both OverUnderflow and Overflow cases.

ahh, `handleOverUnderflow()` will handle both OverUnderflow and Overflow cases, so _handleOverflow() is not necessary here probably.

Comment on attachment 8821670 [details] Bug 1325695 - Handle Overflow and OverUnderflow cases when adjusting the height of autocomplete popup.; update the patch and f? again since _handleOverflow() is not necessary.

The root cause of this issue is about the timing to handle Overflow and UnderOverflow. Overflow and UnderOverflow handling in this patch happens right before the height calculation, and this is the best timing I found so far. This patch makes the height correct even switching between the two fields in test case.

Thanks Sean from get to this!

The insecure login field warning is new in 52, tracking this issue for that release.

Comment on attachment 8821670 [details] Bug 1325695 - Handle Overflow and OverUnderflow cases when adjusting the height of autocomplete popup.; https://reviewboard.mozilla.org/r/100874/#review103190 I'm pretty worried about the performance impact of this on the urlbar. handleOverUnderflow removes the item's max width and then recalculates the widths and max widths of everything in the item. Doing that for every item every time you type in the urlbar, basically, seems bad. Can you make this opt-in for your UI? Like an attribute on the textbox/input or popup. Do handleOverUnderflow only if its value == "true". Also, I may be misunderstanding, but isn't this a problem for only one item in your UI, not all of them? Is it really necessary to call handleOverUnderflow on all items?

Comment on attachment 8821670 [details] Bug 1325695 - Handle Overflow and OverUnderflow cases when adjusting the height of autocomplete popup.; Hi adw, Thanks for your suggestion and I add a class "forceHandleUnderflow" at richlistitem for handling Underflow if needed. Could you help to review it again? I was trying to add the class at [1], but it doesn't work. Is there any simpler way to add a specific class for a binding item? [1] http://searchfox.org/mozilla-central/rev/0f254a30d684796bcc8b6e2a102a0095d25842bb/toolkit/content/widgets/autocomplete.xml#1458

There's one thing that's important for me to understand that I still don't: (In reply to Drew Willcoxon :adw from comment #16) > Also, I may be misunderstanding, but isn't this a problem for only one item > in your UI, not all of them? Is it really necessary to call > handleOverUnderflow on all items? Could you help me understand that please?

Or... your new patch adds a class to the item and not the popup/textbox as I suggested -- is that because, yes, only one item needs handleOverUnderflow()? And the reason you're iterating over all the rows checking for the class -- is that because you're just trying to make that code general and not specific to this particular UI? I was going to comment that I'd rather add an attribute/class on either the popup or the textbox, not the items, so that the check is O(1) instead of O(number of items). But if the reason you're checking all the items is to be general about it, I think that's a fair thing to do.

(In reply to Drew Willcoxon :adw from comment #20) > There's one thing that's important for me to understand that I still don't: > > (In reply to Drew Willcoxon :adw from comment #16) > > Also, I may be misunderstanding, but isn't this a problem for only one item > > in your UI, not all of them? Is it really necessary to call > > handleOverUnderflow on all items? > > Could you help me understand that please? Sorry that I miss this question. The first version patch did apply `handleOverUnderflow` to all items, but the second one only apply it to the item with `forceHandleUnderflow`. (In reply to Drew Willcoxon :adw from comment #21) > Or... your new patch adds a class to the item and not the popup/textbox as I > suggested -- is that because, yes, only one item needs > handleOverUnderflow()? And the reason you're iterating over all the rows > checking for the class -- is that because you're just trying to make that > code general and not specific to this particular UI? Yes, I think the general design will be useful for other items need to handle the text wrap case as well. (like the new design of auto form fill, bug 1326141)

For the first question above, `handleOverUnderflow` will be called when the item is with `forceHandleUnderflow` now.

Comment on attachment 8821670 [details] Bug 1325695 - Handle Overflow and OverUnderflow cases when adjusting the height of autocomplete popup.; OK, thanks for explaining. Before landing, could you please add a code comment that describes what the purpose of that for-loop is?

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b30209ce54bc Handle Overflow and OverUnderflow cases when adjusting the height of autocomplete popup.; r=adw

Sean, please request uplift to 52.

Tested on Mac OS X 10.10, Windows 10 x64 and Ubuntu 16.04 with Nighlty 53.0a1 (2017-01-13) and I can confirm the fix, the behaviour that I see is similar with the one from the attachment: "The autocomplete screenshot with the patch." I have 2 questions here: 1. In case that you zoom in or out the browser page, the alert message text size is the same, this is the intended behaviour I think, right? 2. See the below scenario: Open the test case page from the attachment, you see the alert message. Hover over the alert message, text becomes bold When you move the mouse over the textarea the text remains bold. If you do the same action with the keyboard button the text changes to it initial state. (select one field -> click on down arrow key -> click on up arrow key). Please tell me your opinion about this and if you think that a new bug should be filed. Thanks

Comment on attachment 8821670 [details] Bug 1325695 - Handle Overflow and OverUnderflow cases when adjusting the height of autocomplete popup.; Approval Request Comment [Feature/Bug causing the regression]: Insecure login field contextual warning feature [User impact if declined]: User will see an incorrect size dropdown menu when switching between two different width input field. [Is this code covered by automated tests?]: No. [Has the fix been verified in Nightly?]: Yes. [Needs manual test from QE? If yes, steps to reproduce]: 1. Enter a HTTP website with two different size input. 2. Focus on one input and switch to another. User should see the dropdown without scrollbar. [List of other uplifts needed for the feature/fix]: No. [Is the change risky?]: Not really. [Why is the change risky/not risky?]: This patch just reused the current OverUnderflow mechanism and add it to the right place. These slight changes are without any code removing. [String changes made/needed]: None.

Sean can you take a look at ovidiu's questions in comment 29? Thanks.

(In reply to ovidiu boca[:Ovidiu] from comment #29) > Tested on Mac OS X 10.10, Windows 10 x64 and Ubuntu 16.04 with Nighlty > 53.0a1 (2017-01-13) and I can confirm the fix, the behaviour that I see is > similar with the one from the attachment: "The autocomplete screenshot with > the patch." > > I have 2 questions here: > > 1. In case that you zoom in or out the browser page, the alert message text > size is the same, this is the intended behaviour I think, right? I did not intend to design this behavior before. The message should be zoom-in probably. > > 2. See the below scenario: > > Open the test case page from the attachment, you see the alert message. > Hover over the alert message, text becomes bold > When you move the mouse over the textarea the text remains bold. If you do > the same action with the keyboard button the text changes to it initial > state. (select one field -> click on down arrow key -> click on up arrow > key). I guess this is an expected behavior. > Please tell me your opinion about this and if you think that a new bug > should be filed. Thanks Hey MattN, could you see the above comment as well? I am not sure if I gave the correct comment. Thank you.

(In reply to Sean Lee [:seanlee][:weilonge] from comment #32) > (In reply to ovidiu boca[:Ovidiu] from comment #29) > > Tested on Mac OS X 10.10, Windows 10 x64 and Ubuntu 16.04 with Nighlty > > 53.0a1 (2017-01-13) and I can confirm the fix, the behaviour that I see is > > similar with the one from the attachment: "The autocomplete screenshot with > > the patch." > > > > I have 2 questions here: > > > > 1. In case that you zoom in or out the browser page, the alert message text > > size is the same, this is the intended behaviour I think, right? > I did not intend to design this behavior before. The message should be > zoom-in probably. This is orthogonal to this feature. We didn't zoom before this work either and don't for form history. I don't have strong feelings about whether it should zoom or not but it wouldn't be a password manager bug and wouldn't be part of this project. > > 2. See the below scenario: > > > > Open the test case page from the attachment, you see the alert message. > > Hover over the alert message, text becomes bold > > When you move the mouse over the textarea the text remains bold. If you do > > the same action with the keyboard button the text changes to it initial > > state. (select one field -> click on down arrow key -> click on up arrow > > key). > I guess this is an expected behavior. Yes, this is expected behaviour and is how all of our autocomplete widgets work. > > Please tell me your opinion about this and if you think that a new bug > > should be filed. Thanks No, I think it's the expected behaviour. The "hover" state and "selected" state are two different things. I would rather bug reports that aren't changes from this feature weren't reported in bugs about this feature as it's distracting. You can easily compare to form history behaviour before reporting.

I think we're good for uplift now.

Comment on attachment 8821670 [details] Bug 1325695 - Handle Overflow and OverUnderflow cases when adjusting the height of autocomplete popup.; fix wrapping of autocomplete popup, aurora52+

verified as fixed on 52.0a2 20170122004006 on Ubuntu 16.04, Windows 10 and Mac OSX 10.10.