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

Status: RESOLVED FIXED

(Toolkit :: Form Manager, enhancement, P3)

Description

[Ray Lin[:ralin]]

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.

Comment 1

[Ray Lin[:ralin]]

More context, this only happens when previewing <select>.

Comment 2

[Ray Lin[:ralin]]

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.

Comment 3

[Ray Lin[:ralin]]

Attachment: Bug 1382189 - Persist preview value in PressState for select element to ensure the text is consistent after frame re-construction.

Review commit: https://reviewboard.mozilla.org/r/159140/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/159140/

Comment 4

[Ray Lin[:ralin]]

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 5

[Cameron McCormack (:heycam)]

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?

Comment 6

[Ray Lin[:ralin]]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/159140/diff/1-2/

Comment 7

[Ray Lin[:ralin]]

(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

Comment 8

[Ray Lin[:ralin]]

the issues are fixed, thanks.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c82858285f0b