Closed Bug 1340488 Opened 3 years ago Closed 3 years ago

Add a chrome-only API to preview the option to be auto-selected in a <select>

Categories

(Core :: Layout: Form Controls, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: MattN, Assigned: ralin)

References

()

Details

(Whiteboard: [form autofill:M3])

User Story

Requirements
* chrome-only API to set and reset the previewed text
** e.g. select.mozPreviewValue = "foo@example.com"; // set the previewed text
*** I'm not sure if we want to enforce that the value matches the value of one of the <option>s.
** select.mozPreviewValue = ""; // Reset to the state of no previewed text
* While there is a non-empty value for the preview value, the preview text appears in anonymous content in the same position as selected option's label would. The selected option's label text doesn't appear.
* The previewed text doesn't affect the validity state of the input
* The preview text can be styled via a CSS pseudo element.
** I believe the spec currently only requires `color`.
* The anonymous content shouldn't be detectable by unprivileged content in any way e.g. drawImage or CSS selectors like :empty.
* No DOM events should fire when the API is called
* No mutation observers should be notified when the API is called
* .value doesn't change when the API is called

Attachments

(1 file)

+++ This bug was initially created as a clone of Bug #1340483 +++

When an autocomplete entry is selected/highlighted in an autocomplete dropdown with Form Autofill, all relevant fields that will be autofilled should preview what their values will be upon confirming the selection (e.g. clicking the left mouse button on the autocomplete row or hitting enter).

For both privacy and compatibility, the previewed values shouldn't be exposed to the web page until the user confirms the selection. This means that no DOM events will fire on the <input> as the autocomplete selection changes causing previewed values to change. It also means that accessing .value on the input will return it's value from before previewing.

This bug can just handle adding the anonymous content to render the text that would be filled. We will also probably need to be able to style the text using a pseudo-element (the UX spec has it at a lower opacity or lighter color?) so we may want to see if other browsers already have a name to re-use. Styling the background of the previewed field will be left to bug 740979.

We should be careful about privacy leaks of the previewed data e.g. not allowing the anonymous content to capture and drawn to a canvas untainted and ensure that CSS can't reveal the values either (e.g. using a technique like :visited).

UX Spec: https://mozilla.invisionapp.com/share/TM8GRODVH#/screens/185446465
Our believe our spec matches Chrome behaviour btw.
Assignee: nobody → ralin
Status: NEW → ASSIGNED
Whiteboard: [form autofill] → [form autofill:MVP]
Whiteboard: [form autofill:MVP] → [form autofill:M3]
Hi Cameron,

I initially thought we could do something similar to <input> in <select>, via adding new anonymous div. However, after a while investigation and trying, the layout construction seems more complicated than my expectation... so I ended up changing  the text of textNode of <select> directly, though still not sure if this way is on the right track. Could you kindly give me some feedback about current patch? Thank you so much :-)
Comment on attachment 8864036 [details]
Bug 1340488 - Add a chrome-only previewValue attribute to <select> for showing preview text.

https://reviewboard.mozilla.org/r/135754/#review139096

This generally looks fine, and I think it is preferable to adding a new text node for the preview text.  Some comments below on managing the display string.

::: layout/forms/nsComboboxControlFrame.cpp:979
(Diff revision 1)
> +  // If preview value is empty, we restore the display to selected option
> +  if (previewValue.IsEmpty()) {
> +    RedisplaySelectedText();
> +    return;
> +  }
> +
> +  if (mDisplayContent) {
> +    mRedisplayTextEvent.Revoke();
> +
> +    NS_ASSERTION(!nsContentUtils::IsSafeToRunScript(),
> +                 "If we happen to run our redisplay event now, we might kill "
> +                 "ourselves!");
> +
> +    RefPtr<RedisplayTextEvent> event = new RedisplayTextEvent(this, true);
> +    mRedisplayTextEvent = event;
> +    nsContentUtils::AddScriptRunner(event);
> +  }
> +

If we use mDisplayedOptionText to store the text that should be displayed, regardless of whether it comes from an <option> or mPreviewText, then I think it could simplify things.  For example here we would be able to just call RedisplaySelectedText (though maybe that should then be renamed to RedisplaySelectedTextOrPreview, or something like that).

It should also mean that we don't need to store the mShowPreview boolean on the RedisplayTextEvent.
Attachment #8864036 - Flags: review?(cam)
Comment on attachment 8864036 [details]
Bug 1340488 - Add a chrome-only previewValue attribute to <select> for showing preview text.

https://reviewboard.mozilla.org/r/135754/#review139104

::: layout/forms/nsComboboxControlFrame.cpp:1064
(Diff revision 1)
>    // into the correct parent (mDisplayFrame). See bug 282607.
>    NS_PRECONDITION(!mInRedisplayText, "Nested RedisplayText");
>    mInRedisplayText = true;
>    mRedisplayTextEvent.Forget();
>  
> -  ActuallyDisplayText(true);
> +  nsAutoString actualDisplayText;

This could just be nsString, by the way, since we don't need to use the stack storage that nsAutoString provides, as we're just assigning either mPreviewText or mDisplayedOptionText, and it'll share their buffer internally.
Attachment #8864036 - Flags: feedback+
Glad to have your rapid feedback \o/

> I think it is preferable to adding a new text node for the preview text.
I guess in addition to adding a text node to anonymous content, we would need to modify frame construction[0]. I forgot what I've tried on this, but it turned out that visually I just appended a new text node after the original one. If you don't mind, could you give me some clue how to make new text node display over original frame?   


> If we use mDisplayedOptionText to store the text that should be displayed, regardless of whether it comes from an <option> or 
> mPreviewText, then I think it could simplify things.  For example here we would be able to just call RedisplaySelectedText 
> (though maybe that should then be renamed to RedisplaySelectedTextOrPreview, or something like that).
> 
> It should also mean that we don't need to store the mShowPreview boolean on the RedisplayTextEvent.
Yeah, I agree. Also, the code would be more elegant if we don't need to pass mShowPreview through those methods.


> This could just be nsString, by the way, since we don't need to use the stack storage that nsAutoString provides, as we're just > assigning either mPreviewText or mDisplayedOptionText, and it'll share their buffer internally.
I didn't know that, learned a valuable lesson, thank you :)


will update the patch soon, thanks again!


[0] https://dxr.mozilla.org/mozilla-central/rev/a748acbebbde373a88868dc02910fb2bc5e6a023/layout/forms/nsComboboxControlFrame.cpp#1354-1386
(In reply to Cameron McCormack (:heycam) from comment #3)

> If we use mDisplayedOptionText to store the text that should be displayed,
> regardless of whether it comes from an <option> or mPreviewText, then I
> think it could simplify things.  For example here we would be able to just
> call RedisplaySelectedText (though maybe that should then be renamed to
> RedisplaySelectedTextOrPreview, or something like that).
Since we need to explicitly pass index to RedisplaySelectedText, so I guess instead of just calling RedisplaySelectedText, I'd like to create another method: DispatchRedisplayTextEvent that take into account to dispatch event for both SetPreviewText and RedisplaySelectedText.

> 
> It should also mean that we don't need to store the mShowPreview boolean on
> the RedisplayTextEvent.
I've removed those in this revision.

Could you help me to review this again? Thank you.
What's the expected behavior if the currently selected index changes while the autofill controller has set preview text on the <select>?  For example, maybe the page is doing:

  setInterval(function() {
    select.selectedIndex = Math.floor(Math.random() * select.options.length);
  }, 1000);

and from the code it looks like we will call RedisplayText with the new index, which will cause the newly selected option to be displayed instead of the preview text.

Currently, RedisplayText always sets mDisplayedIndex to the value that is passed in as its argument.  So I think we should remove the argument, and always set mDisplayedIndex just before all of the RedisplayText callers.  Then, we can make RedisplayText be the single place where we make the decision about whether to show the preview text or option text.

I see in nsComboboxControlFrame::Reflow that we try to avoid calling RedisplayText if the text to be shown isn't going to change.  I think we should move that optimization into the RedisplayText function itself, after it has decided what the actual string to display is.
(In reply to Cameron McCormack (:heycam) from comment #8)
> What's the expected behavior if the currently selected index changes while
> the autofill controller has set preview text on the <select>?  For example,
> maybe the page is doing:
> 
>   setInterval(function() {
>     select.selectedIndex = Math.floor(Math.random() * select.options.length);
>   }, 1000);
> 
> and from the code it looks like we will call RedisplayText with the new
> index, which will cause the newly selected option to be displayed instead of
> the preview text. 
I'm not sure which is better, either respect the new item or do nothing are fine to me. I've just brief this question to Luke, and he said he prefer to show preview text until we unset it. So, I think we should go the latter option.


> Currently, RedisplayText always sets mDisplayedIndex to the value that is
> passed in as its argument.  So I think we should remove the argument, and
> always set mDisplayedIndex just before all of the RedisplayText callers. 
I guess we can leave mDisplayedIndex as-is and set the mPreviewText then call RedisplayText?

> Then, we can make RedisplayText be the single place where we make the
> decision about whether to show the preview text or option text.
Here would be like: 

// Get the text to display
if (!mPreviewText.IsEmpty()) {
  mDisplayedOptionTextOrPreview = mPreviewText;
} else if (aIndex != -1) {
  mListControlFrame->GetOptionText(aIndex, mDisplayedOptionTextOrPreview);
} else {
  mDisplayedOptionTextOrPreview.Truncate();
}

> 
> I see in nsComboboxControlFrame::Reflow that we try to avoid calling
> RedisplayText if the text to be shown isn't going to change.  I think we
> should move that optimization into the RedisplayText function itself, after
> it has decided what the actual string to display is.
Got it. We could decide whether to dispatch RedisplayTextEvent by comparing new/previous mDisplayedOptionTextOrPreview.


Thanks :)
(In reply to Ray Lin[:ralin] from comment #9)
> > Currently, RedisplayText always sets mDisplayedIndex to the value that is
> > passed in as its argument.  So I think we should remove the argument, and
> > always set mDisplayedIndex just before all of the RedisplayText callers. 
> I guess we can leave mDisplayedIndex as-is and set the mPreviewText then
> call RedisplayText?

In SetPreviewText?  Yes, I think that's right.

> > Then, we can make RedisplayText be the single place where we make the
> > decision about whether to show the preview text or option text.
> Here would be like: 
> 
> // Get the text to display
> if (!mPreviewText.IsEmpty()) {
>   mDisplayedOptionTextOrPreview = mPreviewText;
> } else if (aIndex != -1) {
>   mListControlFrame->GetOptionText(aIndex, mDisplayedOptionTextOrPreview);
> } else {
>   mDisplayedOptionTextOrPreview.Truncate();
> }

Yes, that looks right too.  Thanks!
Comment on attachment 8864036 [details]
Bug 1340488 - Add a chrome-only previewValue attribute to <select> for showing preview text.

https://reviewboard.mozilla.org/r/135754/#review140494

r=me with that fixed.  Also, don't forget to get a DOM peer's review for the .webidl change.

::: layout/forms/nsComboboxControlFrame.cpp:848
(Diff revision 3)
>      selectedIndex = mDisplayedIndex;
>    }
>    if (selectedIndex != -1) {
>      mListControlFrame->GetOptionText(selectedIndex, selectedOptionText);
>    }
> -  if (mDisplayedOptionText != selectedOptionText) {
> +  RedisplayText();

It's not clear to me whether mDisplayedIndex is always set correctly before here.  In the "if (!mDroppedDown)" branch above, can you set mDisplayedIndex to selectedIndex?

Or really, I think we can just get rid of selectedIndex, and just set mDisplayedIndex appropriately.

::: layout/forms/nsComboboxControlFrame.cpp:996
(Diff revision 3)
> +  }
> +  else if (mDisplayedIndex != -1) {

Nit: no newline after "}".
Attachment #8864036 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #12)
> Comment on attachment 8864036 [details]
> Bug 1340488 - Add a chrome-only previewValue attribute to <select> for
> showing preview text.
> 
> https://reviewboard.mozilla.org/r/135754/#review140494
> 
> r=me with that fixed.  Also, don't forget to get a DOM peer's review for the
> .webidl change.
> 
> ::: layout/forms/nsComboboxControlFrame.cpp:848
> (Diff revision 3)
> >      selectedIndex = mDisplayedIndex;
> >    }
> >    if (selectedIndex != -1) {
> >      mListControlFrame->GetOptionText(selectedIndex, selectedOptionText);
> >    }
> > -  if (mDisplayedOptionText != selectedOptionText) {
> > +  RedisplayText();
> 
> It's not clear to me whether mDisplayedIndex is always set correctly before
> here.  In the "if (!mDroppedDown)" branch above, can you set mDisplayedIndex
> to selectedIndex?
> 
> Or really, I think we can just get rid of selectedIndex, and just set
> mDisplayedIndex appropriately.
Maybe it would be safer to set selectIndex as well in case somewhere using it after that, I guess

Not sure which coding style is more proper for DOM code, to separate like: 
selectedIndex = mListControlFrame->GetSelectedIndex();
mDisplayedIndex = selectedIndex;
or combine those into one line?
mDisplayedIndex = selectedIndex = mListControlFrame->GetSelectedIndex();

Thanks.
(In reply to Ray Lin[:ralin] from comment #13)
> Maybe it would be safer to set selectIndex as well in case somewhere using
> it after that, I guess

It looks like nothing else uses selectedIndex, and it's only used as the value to pass to RedisplayText, which is why I think it might be worth simplifying this block of code to:

  if (!mDroppedDown) {
    mDisplayedIndex = mListControlFrame...
  } else {
    // leave mDisplayedIndex at its current value
  }
  RedisplayText();
    
> Not sure which coding style is more proper for DOM code, to separate like: 
> selectedIndex = mListControlFrame->GetSelectedIndex();
> mDisplayedIndex = selectedIndex;
> or combine those into one line?
> mDisplayedIndex = selectedIndex = mListControlFrame->GetSelectedIndex();

Separate lines are preferred.  But if we simplify the code we don't need to worry about that. :-)
Thank you so much Cameron :-)
Hi :baku

Since I made some changes that expose a new chrome-only attribute for internal usage in <select> .webidl, could you help me to review it? Thank you.
Comment on attachment 8864036 [details]
Bug 1340488 - Add a chrome-only previewValue attribute to <select> for showing preview text.

https://reviewboard.mozilla.org/r/135754/#review140536

r+ for the dom part.

::: layout/forms/nsComboboxControlFrame.h:277
(Diff revision 4)
>    void CheckFireOnChange();
>    void FireValueChangeEvent();
> -  nsresult RedisplayText(int32_t aIndex);
> +  nsresult RedisplayText();
>    void HandleRedisplayTextEvent();
>    void ActuallyDisplayText(bool aNotify);
> +  void GetPreviewText(nsAString& aValue) {

{ in a new line
Attachment #8864036 - Flags: review?(amarchesini) → review+
Thanks for the review!
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/656b4466e33b
Add a chrome-only previewValue attribute to <select> for showing preview text. r=baku,heycam
Keywords: checkin-needed
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3f90e402ef50
Backed out changeset 656b4466e33b for Bustage in nsComboboxControlFrame.h
Sorry I didn't notice the `override` are missing...

Updated the patch and tested on local machine without compile warnings. Thanks.
Flags: needinfo?(ralin)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1c0c17bd0279
Add a chrome-only previewValue attribute to <select> for showing preview text. r=baku,heycam
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1c0c17bd0279
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Flags: qe-verify-
Duplicate of this bug: 1333370
You need to log in before you can comment on or make changes to this bug.