Closed Bug 1385788 Opened 6 years ago Closed 6 years ago

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

Categories

(Toolkit :: Form Manager, enhancement, P3)

Product:
Toolkit
Component:
Form Manager
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: ralin, Assigned: ralin)

References

Details

(Whiteboard: [form autofill:MVP])

Attachments

(1 file)

Bug 1385788 - Move the ownership of preview value from PresState to HTMLSelectElement.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details 
It would be better to persist the previeValue in DOM object(HTMLSelectElement) as that's more similar to what we're doing for <input>.
Blocks: 1409366
Blocks: 1409350
No longer blocks: 1409366
Assignee: nobody → ralin
Status: NEW → ASSIGNED
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
Flags: needinfo?(cam)
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.
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.
Flags: needinfo?(cam)
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.
Whiteboard: [form autofill] → [form autofill:MVP]
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.
Attachment #8921767 - Flags: review?(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.
Flags: needinfo?(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).
Flags: needinfo?(emilio)
OK, but does that mean that the previewValue does not (and did not) survive reframe for <input>?
Flags: needinfo?(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.
Flags: needinfo?(ralin)
> 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?
(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.
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
(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.
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).
Flags: needinfo?(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.
Flags: needinfo?(ralin)
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.
(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
See Also: → 1412241
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
Attachment #8921767 - Flags: review?(bzbarsky) → review+
Sorry, I had missed the review request somehow....  Looks good!
Thanks for reviewing :)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/52e92df6bf6a
Move the ownership of preview value from PresState to HTMLSelectElement. r=bz
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/52e92df6bf6a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.