[Form Autofill] Setting manuallyManagedState bit might trigger frame re-construction and erase the preview text while previewing

RESOLVED FIXED in Firefox 56

Status

()

Toolkit
Form Manager
P3
normal
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: ralin, Assigned: ralin)

Tracking

(Depends on: 1 bug)

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [form autofill:M4], URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

3 months ago
We should persist the previewText pointer in select element instead of in frame. So that any frame reconstruction won't erase the existing preview text.
(Assignee)

Comment 1

3 months ago
More context, this only happens when previewing <select>.
(Assignee)

Comment 2

3 months ago
Confirmed this issue by printing the message in every constructor/destructor and found that setting manuallyManagedState bit does trigger re-construction. I'll fix this with the similar way we've implemented for the input element.
(Assignee)

Updated

3 months ago
Assignee: nobody → ralin
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Assignee)

Comment 4

3 months ago
Hey, Cameron

So glad with all your help providing me different approaches for this issue. I tried to persist the data by SaveState, and Restore the data once the frame being re-constructed, and it does fix the problem. Could you help me to review this patch, and see if any problem I may not think through? Thanks again.
Comment on attachment 8888198 [details]
Bug 1382189 - Persist preview value in PressState for select element to ensure the text is consistent after frame re-construction.

https://reviewboard.mozilla.org/r/159140/#review165020

I think this looks good, thanks!  Although I think it would be a little better to store the preview string on the HTMLSelectElement object (since it would be more similar to what we're doing for text input elements, where the editor state is closely related to the DOM object, not the frame), but this is OK too.

::: layout/base/nsPresState.h:114
(Diff revision 1)
>    bool GetDroppedDown() const
>    {
>      return mDroppedDown;
>    }
>  
> +  void SetPreviewText(const nsAString& aValue) {

Nit: I think the coding style requires braces to go on the next line, even for inline functions like this.

::: layout/forms/nsComboboxControlFrame.cpp:1694
(Diff revision 1)
>    ShowList(aState->GetDroppedDown()); // might destroy us
> +  aState->GetPreviewText(mPreviewText);

I worry about the comment there on the previous line that says "might destroy us".  Can you move the GetPreviewText call one line up to avoid that?
Attachment #8888198 - Flags: review?(cam) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 7

3 months ago
(In reply to Cameron McCormack (:heycam) from comment #5)
> Comment on attachment 8888198 [details]
> Bug 1382189 - Persist preview value in PressState for select element to
> ensure the text is consistent after frame re-construction.
> 
> https://reviewboard.mozilla.org/r/159140/#review165020
> 
> I think this looks good, thanks!  Although I think it would be a little
> better to store the preview string on the HTMLSelectElement object (since it
> would be more similar to what we're doing for text input elements, where the
> editor state is closely related to the DOM object, not the frame), but this
> is OK too.
yeah, I agree. It make more sense to store it on HTMLSelectElement, learned a lession. However, I feel we kinda under the pressure from QA sign-off and the upcoming merge date, so I'd like to go this approach first and file a followup to refine it after the point after form autofill is shipped. Thanks for your rapid review :D
(Assignee)

Comment 8

3 months ago
the issues are fixed, thanks.
Keywords: checkin-needed

Comment 9

3 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c82858285f0b
Persist preview value in PressState for select element to ensure the text is consistent after frame re-construction. r=heycam
Keywords: checkin-needed

Comment 10

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c82858285f0b
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(Assignee)

Updated

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