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

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, enhancement)

Description

[Matthew N. [:MattN]]

+++ 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.

Comment 1

[Ray Lin[:ralin]]

Attachment: Bug 1340488 - Add a chrome-only previewValue attribute to <select> for showing preview text.

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

Comment 2

[Ray Lin[:ralin]]

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 3

[Cameron McCormack (:heycam)]

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.

Comment 4

[Cameron McCormack (:heycam)]

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.

Comment 5

[Ray Lin[:ralin]]

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

Comment 6

[Ray Lin[:ralin]]

Comment on attachment 8864036 [details]
Bug 1340488 - Add a chrome-only previewValue attribute to <select> for showing preview text.

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

Comment 7

[Ray Lin[:ralin]]

(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.

Comment 8

[Cameron McCormack (:heycam)]

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.

Comment 9

[Ray Lin[:ralin]]

(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 :)

Comment 10

[Cameron McCormack (:heycam)]

(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 11

[Ray Lin[:ralin]]

Comment on attachment 8864036 [details]
Bug 1340488 - Add a chrome-only previewValue attribute to <select> for showing preview text.

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

Comment 12

[Cameron McCormack (:heycam)]

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 "}".

Comment 13

[Ray Lin[:ralin]]

(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.

Comment 14

[Cameron McCormack (:heycam)]

(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. :-)

Comment 15

[Ray Lin[:ralin]]

Comment on attachment 8864036 [details]
Bug 1340488 - Add a chrome-only previewValue attribute to <select> for showing preview text.

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

Comment 16

[Ray Lin[:ralin]]

Thank you so much Cameron :-)

Comment 17

[Ray Lin[:ralin]]

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 18

[Andrea Marchesini [:baku]]

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

Comment 19

[Ray Lin[:ralin]]

Comment on attachment 8864036 [details]
Bug 1340488 - Add a chrome-only previewValue attribute to <select> for showing preview text.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135754/diff/4-5/

Comment 20

[Ray Lin[:ralin]]

Thanks for the review!

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/656b4466e33b
Add a chrome-only previewValue attribute to <select> for showing preview text. r=baku,heycam

Comment 22

[Carsten Book [:Tomcat]]

backed out for https://treeherder.mozilla.org/logviewer.html#?job_id=97685306&repo=autoland bustage

Comment 23

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3f90e402ef50
Backed out changeset 656b4466e33b for Bustage in nsComboboxControlFrame.h

Comment 24

[Ray Lin[:ralin]]

Comment on attachment 8864036 [details]
Bug 1340488 - Add a chrome-only previewValue attribute to <select> for showing preview text.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135754/diff/5-6/

Comment 25

[Ray Lin[:ralin]]

Sorry I didn't notice the `override` are missing...

Updated the patch and tested on local machine without compile warnings. Thanks.

Comment 26

[Ray Lin[:ralin]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=016b38ffc564bbb2aaffbeef5d2e541d38a3cef4

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1c0c17bd0279
Add a chrome-only previewValue attribute to <select> for showing preview text. r=baku,heycam

Comment 28

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/1c0c17bd0279