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

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
4 months ago
2 months ago

People

(Reporter: lchang, Assigned: lchang)

Tracking

(Blocks: 1 bug)

Trunk
mozilla55
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [form autofill:M2])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

4 months ago
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)

Updated

4 months ago
Assignee: nobody → lchang
Status: NEW → ASSIGNED
Whiteboard: [form autofill:MVP] → [form autofill:M2]
Comment hidden (mozreview-request)
(Assignee)

Comment 2

4 months ago
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 4

4 months ago
mozreview-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/#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+
(Assignee)

Comment 5

4 months ago
mozreview-review-reply
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.
(Assignee)

Comment 6

4 months ago
mozreview-review-reply
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.
(Assignee)

Comment 7

4 months ago
Hi Cameron & Matt,

Thanks a lot for your review. I'll update it soon.
Comment hidden (mozreview-request)

Comment 9

4 months ago
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

Comment 10

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/afdbbb354869
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Blocks: 1364354

Updated

2 months ago
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.