1385788 - [Form Autofill] Persist <select> previewValue in HTMLSelectElement instead of PressState

Status: RESOLVED FIXED

(Toolkit :: Form Manager, enhancement, P3)

Description

[Ray Lin[:ralin]]

It would be better to persist the previeValue in DOM object(HTMLSelectElement) as that's more similar to what we're doing for <input>.

Comment 1

[Ray Lin[:ralin]]

Hi Cameron,

I found the previewValue wasn't restored for <select> in some cases, because the nsFrameManager couldn't get the state from hash[0] after frame re-construction when form autofill previewing.

And our preview is consist of two steps:
1. setting .previewValue to <select>
2. addManuallyManagedState to apply yellow background using CSS filter on <select>. (this is what makes frame re-construct, I guess)

It weird that the save/restore result varies in these two websites:
- payment panel of sears.com (Bug 1409350)
- our demo page, https://luke-chang.github.io/autofill-demo/textarea_select.html

In sears.com, the <select> are two expiration date fields[1]. They might look like custom compound dropdowns but actually a styled <select> inside. The other side, the <select> element in our demo page is a unstyled regular one. I had hard time finding the reason making the restore behave differently...

----
To fix this, not sure what's the correct way, I think we can go two directions:
1. fix save/restore for sears.com's case if it's a bug for StateFrame.
2. as titled said, persist the previewValue in somewhere maybe DOM element <select>

Could you give me some thoughts about these two approaches? Thank you so much :D



[0] http://searchfox.org/mozilla-central/rev/1c4da216e00ac95b38a3f236e010b31cdfaae03b/layout/base/nsFrameManager.cpp#650-654
[1] https://bugzilla.mozilla.org/attachment.cgi?id=8919251

Comment 2

[Ray Lin[:ralin]]

Also, if we chose approach 2, I don't know if it's still appropriate as in https://hg.mozilla.org/releases/mozilla-beta/rev/4d33cf91c8bd, the patch moved the placeholder and preview part from DOM to layout.

Comment 3

[Cameron McCormack (:heycam)]

I won't get a chance to look at specifically what is going wrong with sears.com and the storing of the preview text on the frame before I am out for travel/PTO tomorrow, but I feel like storing the preview text on the DOM element would be better, as we wouldn't have to worry about any frame state restoration issues at all.  I don't think the fact that the anonymous content ownership moved to the frame impacts this.

Comment 4

[Ray Lin[:ralin]]

Attachment: Bug 1385788 - Move the ownership of preview value from PresState to HTMLSelectElement.

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

Comment 5

[Ray Lin[:ralin]]

Hi :bz, 

Nice to meet you over bugzilla :D

In this bug, I have no idea the preview string stored in PressState sometimes was gone after frame re-construction, so as Cameron suggested, in the patch, I move the ownership of the string from frame to DOM. Could you help me to review it? Thanks.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8921767 [details]
Bug 1385788 - Move the ownership of preview value from PresState to HTMLSelectElement.

https://reviewboard.mozilla.org/r/192786/#review198448

I'd like to understand what's going on with the RemoveNewlines thing.

::: commit-message-a124f:1
(Diff revision 1)
> +Bug 1385788 - Move the ownership of preview value from PressState to HTMLSelectElement. r=bz

s/PressState/PresState/

::: layout/forms/nsComboboxControlFrame.cpp
(Diff revision 1)
>  
> -void
> -nsComboboxControlFrame::SetPreviewText(const nsAString& aValue)
> -{
> -  nsAutoString previewValue(aValue);
> -  nsContentUtils::RemoveNewlines(previewValue);

Where does this RemoveNewlines behavior happen now?  I don't see it anywhere.

::: layout/forms/nsComboboxControlFrame.cpp:988
(Diff revision 1)
>  nsComboboxControlFrame::RedisplayText()
>  {
> +  nsString previewValue;
>    nsString previousText(mDisplayedOptionTextOrPreview);
> +
> +  dom::HTMLSelectElement* selectElement = static_cast<dom::HTMLSelectElement*>(GetContent());

"auto*" would be fine here.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Ray is right about one thing.  With the patch in bug 1401706 it looks to me like the <input> previewValue is now stored in the frame and wouldn't survive a reframe.... That seems a bit odd.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #7)
> Ray is right about one thing.  With the patch in bug 1401706 it looks to me
> like the <input> previewValue is now stored in the frame and wouldn't
> survive a reframe.... That seems a bit odd.

I don't think the setup changes significantly in that regard with bug 1401706. Before that bug we removed the preview value in UnbindFromFrame anyway, which was called from nsTextControlFrame::DestroyFrom. (The difference in that bug were other callers to UnbindFromFrame).

Comment 9

[Boris Zbarsky [:bzbarsky]]

OK, but does that mean that the previewValue does not (and did not) survive reframe for <input>?

Comment 10

[Ray Lin[:ralin]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #9)
> OK, but does that mean that the previewValue does not (and did not) survive
> reframe for <input>?

Apologize for I'm not familiar with gecko codebase, but as I know the previewValue did and does survive for <input> regardless of bug 1401706 applied or not.

Comment 11

[Ray Lin[:ralin]]

Comment on attachment 8921767 [details]
Bug 1385788 - Move the ownership of preview value from PresState to HTMLSelectElement.

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

Comment 12

[Boris Zbarsky [:bzbarsky]]

> but as I know the previewValue did and does survive for <input>

I really don't see how that can happen.  How would I test this?

Comment 13

[Ray Lin[:ralin]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #12)
> > but as I know the previewValue did and does survive for <input>
> 
> I really don't see how that can happen.  How would I test this?

For <input>: 
1. create a test page: data:text/html, <input type="text"> and run in non-e10s mode. 
2. Open developer tool to select and set a arbitrary .previewValue to input element. (.previewValue is chrome-only API)
3. apply a style like: filter: grayscale(21%) brightness(88%) contrast(161%) invert(10%) sepia(40%) saturate(206%); (that the style we use for form autofill preview)
4. check if the preview text is still

Not sure I'm testing it right, but the previewValue for <input> seems survived in all our use cases.

Comment 14

[Ray Lin[:ralin]]

Comment on attachment 8921767 [details]
Bug 1385788 - Move the ownership of preview value from PresState to HTMLSelectElement.

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

Comment 15

[Ray Lin[:ralin]]

I'm so sorry, I forgot to mention that we also need to change the initial value of mIsPreviewEnabled to true here:

http://searchfox.org/mozilla-central/rev/dd47bee6468de7e1221b4d006342ad6b9813d0e5/dom/html/HTMLInputElement.cpp#1148

Comment 16

[Ray Lin[:ralin]]

(In reply to Ray Lin[:ralin] from comment #15)
> I'm so sorry, I forgot to mention that we also need to change the initial
> value of mIsPreviewEnabled to true here:
> 
> http://searchfox.org/mozilla-central/rev/
> dd47bee6468de7e1221b4d006342ad6b9813d0e5/dom/html/HTMLInputElement.cpp#1148
To clarify, we set mIsPreviewEnabled to true when we're sure the input is eligible for form autofill, and call http://searchfox.org/mozilla-central/rev/dd47bee6468de7e1221b4d006342ad6b9813d0e5/toolkit/components/satchel/nsFormFillController.cpp#338-370 to enable the ability of showing .previewValue, so if we want to do a test only for .previewValue, I think it might be easier to change the initial value to "true", and go through the steps in comment 13.
(maybe even much easier, we can remove the [ChromeOnly] attribute in .webidl to test in normal developer tool with e10s)

Sorry again if I said something misleading.

Comment 17

[Boris Zbarsky [:bzbarsky]]

OK, so I followed these steps to reproduce:

1) Change the default value of mIsPreviewEnabled to true.
2) Remove [ChromeOnly] from the previewValue webidl.
3) Load this page:

  <input id="i">
  <script>
    onload = function() {
      i.previewValue = "Hey there";
      i.style.display = "block";
    }
  </script>

as I expected, the previewValue disappears.  If I take out the "i.style.display" assignment, it doesn't disappear.  Applying a filter style doesn't trigger a reframe, so doesn't affect things in this case, of course.

I then removed the [ChromeOnly] from the previewValue webidl for HTMLSelectElement, and tried this page:

  <select id="s">
    <option>One</option>
    <option>Two</option>
  </select>
  <script>
    onload = function() {
      s.previewValue = "Three";
      s.style = "filter: grayscale(21%) brightness(88%) contrast(161%) invert(10%) sepia(40%) saturate(206%);";
    }
  </script>

and it shows the preview value (again as expected, since there is no reframe in this case).

I don't know exactly what situations you were testing, but fundamentally storing this state in the frame in any way looks completely broken to me and we should fix it for inputs as well (e.g. by really storing it in the editor state and propagating to the anon content when we are bound to the frame).

Comment 18

[Ray Lin[:ralin]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #17)
> OK, so I followed these steps to reproduce:
> 
> 1) Change the default value of mIsPreviewEnabled to true.
> 2) Remove [ChromeOnly] from the previewValue webidl.
> 3) Load this page:
> 
>   <input id="i">
>   <script>
>     onload = function() {
>       i.previewValue = "Hey there";
>       i.style.display = "block";
>     }
>   </script>
> 
> as I expected, the previewValue disappears.  If I take out the
> "i.style.display" assignment, it doesn't disappear.  Applying a filter style
> doesn't trigger a reframe, so doesn't affect things in this case, of course.
> 
> I then removed the [ChromeOnly] from the previewValue webidl for
> HTMLSelectElement, and tried this page:
> 
>   <select id="s">
>     <option>One</option>
>     <option>Two</option>
>   </select>
>   <script>
>     onload = function() {
>       s.previewValue = "Three";
>       s.style = "filter: grayscale(21%) brightness(88%) contrast(161%)
> invert(10%) sepia(40%) saturate(206%);";
>     }
>   </script>
> 
> and it shows the preview value (again as expected, since there is no reframe
> in this case).
> 
> I don't know exactly what situations you were testing, but fundamentally
> storing this state in the frame in any way looks completely broken to me and
> we should fix it for inputs as well (e.g. by really storing it in the editor
> state and propagating to the anon content when we are bound to the frame).

Seems we never really trigger reframe for <input> in our use case(I thought applying filter does...), so didn't spot the problem. I'll try to fix <input> as well in this bug. Thanks a lot.

Comment 19

[Boris Zbarsky [:bzbarsky]]

I would actually suggest a separate bug for <input> (and textarea, but I expect the same fix to work for both).

filter definitely does not reframe.  See http://searchfox.org/mozilla-central/rev/40e8eb46609dcb8780764774ec550afff1eed3a5/layout/style/nsStyleStruct.cpp#4680-4684 which doesn't include nsChangeHint_ReconstructFrame.

Comment 20

[Ray Lin[:ralin]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #19)

> I would actually suggest a separate bug for <input> (and textarea, but I
> expect the same fix to work for both).
No problem, I'll file another bug for <input> and <textarea>. May we move forward this patch? 

> filter definitely does not reframe.  See
> http://searchfox.org/mozilla-central/rev/
> 40e8eb46609dcb8780764774ec550afff1eed3a5/layout/style/nsStyleStruct.cpp#4680-
> 4684 which doesn't include nsChangeHint_ReconstructFrame.
First time look into the change hint, thanks for the information, learned more from that :D

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8921767 [details]
Bug 1385788 - Move the ownership of preview value from PresState to HTMLSelectElement.

https://reviewboard.mozilla.org/r/192786/#review199008

r=me

Comment 22

[Boris Zbarsky [:bzbarsky]]

Sorry, I had missed the review request somehow....  Looks good!

Comment 23

[Ray Lin[:ralin]]

Thanks for reviewing :)

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/52e92df6bf6a
Move the ownership of preview value from PresState to HTMLSelectElement. r=bz

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/52e92df6bf6a