Closed Bug 1355438 Opened 7 years ago Closed 7 years ago

[Form Autofill] Implement an internal-only pseudo-class for highlighting elements with an autofilled value

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: lchang, Assigned: lchang)

References

(Blocks 1 open bug)

Details

(Whiteboard: [form autofill:M2])

Attachments

(1 file)

We have bug 740979 discussing how to implement the :-moz-autofill pseudo-class. However, the spec of :autofill is not yet clear. In order not to block Form Autofill feature, I'm going to implement an internal-only pseudo-class and only apply it to Form Autofill for now. We can improve this internal-only pseudo-class in bug 740979 once we have more details.
Assignee: nobody → lchang
Status: NEW → ASSIGNED
Whiteboard: [form autofill:MVP] → [form autofill:M2]
Hi Cameron,

Could you take a look at this patch? Thanks.
Comment on attachment 8856938 [details]
Bug 1355438 - [Form Autofill] Implement an internal-only pseudo-class for highlighting elements with an autofilled value.

https://reviewboard.mozilla.org/r/128862/#review131420

::: layout/style/res/html.css:905
(Diff revision 1)
> +:-moz-autofill {
> +  filter: grayscale(21%) brightness(88%) contrast(161%) invert(10%) sepia(40%) saturate(206%);
> +}

Perhaps this should be in layout/style/res/forms.css but really I think we will want it in the formautofill system add-on so we can tweak it.

::: layout/style/res/html.css:905
(Diff revision 1)
>  }
>  ruby, rb, rt, rtc {
>    unicode-bidi: isolate;
>  }
> +
> +:-moz-autofill {

Cam will know better about the performance of this selector, but if it would make it more performant we could add element names like:
```css
input:-moz-autofill,
select:-moz-autofill,
textarea:-moz-autofill {
```
Comment on attachment 8856938 [details]
Bug 1355438 - [Form Autofill] Implement an internal-only pseudo-class for highlighting elements with an autofilled value.

https://reviewboard.mozilla.org/r/128862/#review134660

::: dom/events/EventStates.h:291
(Diff revision 1)
>  #define NS_EVENT_STATE_FOCUS_WITHIN NS_DEFINE_EVENT_STATE_MACRO(43)
>  // Element is ltr (for :dir pseudo-class)
>  #define NS_EVENT_STATE_LTR NS_DEFINE_EVENT_STATE_MACRO(44)
>  // Element is rtl (for :dir pseudo-class)
>  #define NS_EVENT_STATE_RTL NS_DEFINE_EVENT_STATE_MACRO(45)
>  

Please add a comment here saying what this event state bit means.

::: layout/style/res/html.css:284
(Diff revision 1)
>    /* Set hidden if we have 'frame' or 'rules' attribute.
>       Set it on all sides when we do so there's more consistency
>       in what authors should expect */
>  
>    /* Put this first so 'border' and 'frame' rules can override it. */
> -table[rules] { 
> +table[rules] {

Might be that your editor did this automatically, but if you want to make these stylistic changes that aren't related to :-moz-auto-fill, could you please put them in a separate patch.

::: layout/style/res/html.css:905
(Diff revision 1)
>  }
>  ruby, rb, rt, rtc {
>    unicode-bidi: isolate;
>  }
> +
> +:-moz-autofill {

It should be fine either way.  When an event state bit is added or removed from the element, we'll just post a restyle for that element.  Since we never set this state bit on other elements, we won't do any wasteful restyling or selector matching on other elements.

::: layout/style/res/html.css:905
(Diff revision 1)
> +:-moz-autofill {
> +  filter: grayscale(21%) brightness(88%) contrast(161%) invert(10%) sepia(40%) saturate(206%);
> +}

I agree forms.css would be a better location.
Attachment #8856938 - Flags: review?(cam) → review+
Comment on attachment 8856938 [details]
Bug 1355438 - [Form Autofill] Implement an internal-only pseudo-class for highlighting elements with an autofilled value.

https://reviewboard.mozilla.org/r/128862/#review131420

> Perhaps this should be in layout/style/res/forms.css but really I think we will want it in the formautofill system add-on so we can tweak it.

I haven't found a way to load a CSS from our system add-on without modifying the content, so I'll move it to forms.css first.
Comment on attachment 8856938 [details]
Bug 1355438 - [Form Autofill] Implement an internal-only pseudo-class for highlighting elements with an autofilled value.

https://reviewboard.mozilla.org/r/128862/#review134660

> Might be that your editor did this automatically, but if you want to make these stylistic changes that aren't related to :-moz-auto-fill, could you please put them in a separate patch.

Since I'm going to move the CSS to forms.css, I'll leave html.css as-is. However, thanks for pointing it out. I'll put them in a separate patch next time.
Hi Cameron & Matt,

Thanks a lot for your review. I'll update it soon.
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/afdbbb354869
[Form Autofill] Implement an internal-only pseudo-class for highlighting elements with an autofilled value. r=heycam
https://hg.mozilla.org/mozilla-central/rev/afdbbb354869
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.