Closed Bug 1382189 Opened 2 years ago Closed 2 years ago

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

Categories

(Toolkit :: Form Manager, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: ralin, Assigned: ralin)

References

(Depends on 1 open bug, )

Details

(Whiteboard: [form autofill:M4])

Attachments

(1 file)

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.
More context, this only happens when previewing <select>.
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: nobody → ralin
Status: NEW → ASSIGNED
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+
(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
the issues are fixed, thanks.
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/c82858285f0b
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Depends on: 1385788
Depends on: 1412241
You need to log in before you can comment on or make changes to this bug.