[Doorhanger] Alternative main action button styling for form autofill

VERIFIED FIXED in Firefox 56

Status

()

Firefox
Site Identity and Permission Panels
P2
normal
VERIFIED FIXED
5 months ago
2 months ago

People

(Reporter: steveck, Assigned: steveck)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 verified, firefox57 verified)

Details

(Whiteboard: [form autofill:MVP])

MozReview Requests

()

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

Attachments

(3 attachments)

(Assignee)

Description

5 months ago
It's a follow up from bug 1374508 that UX would prefer the original button styling instead of default main action button styling(1). Although we could hack the button to remove the button's "default" attribute while doorhanger shown, user will perceive the button flashing to the new style. In order to prevent flashing
 issue, there seems no other solution than fixing this in PopupNotification.

Hi Paolo, it seems like we might need to add another option like "setButtonStyle" or something to change the button style, but having way to much trivial options sound not a best solution. I'm thinking that maybe we can have a function like customization function for the options. We could get the notification element and modify it before it append to parent, wdyt?

(1) https://mozilla.invisionapp.com/share/AP8TFZ22G#/screens/185446463
(Assignee)

Updated

5 months ago
Flags: needinfo?(paolo.mozmail)

Comment 1

5 months ago
I think that whatever API is implemented should make the caller independent from the structural details of the panel as much as possible. Johann might be a good reviewer to design this change to PopupNotifications.
Flags: needinfo?(paolo.mozmail) → needinfo?(jhofmann)
(Assignee)

Comment 2

5 months ago
(In reply to :Paolo Amadini from comment #1)
> I think that whatever API is implemented should make the caller independent
> from the structural details of the panel as much as possible. 

Hi Johann,

I was thinking about adding adding a customization function provided by options to here(1), but I realize that it might not a good thing for user having such flexibility. Do you think it's fine to add another option like "buttonStyle" for this specific case?

(1) https://dxr.mozilla.org/mozilla-central/rev/cc903e3f61894e60c3b0efaf05e9a446d1d85888/toolkit/modules/PopupNotifications.jsm#875
(In reply to Steve Chung [:steveck] from comment #2)
> (In reply to :Paolo Amadini from comment #1)
> > I think that whatever API is implemented should make the caller independent
> > from the structural details of the panel as much as possible. 
> 
> Hi Johann,
> 
> I was thinking about adding adding a customization function provided by
> options to here(1), but I realize that it might not a good thing for user
> having such flexibility. Do you think it's fine to add another option like
> "buttonStyle" for this specific case?
> 
> (1)
> https://dxr.mozilla.org/mozilla-central/rev/
> cc903e3f61894e60c3b0efaf05e9a446d1d85888/toolkit/modules/PopupNotifications.
> jsm#875

Yeah, I'd try to keep the customization minimal and avoid over-engineering this for now. How about a "highlight" attribute in mainAction that can be true or false depending on whether it should be blue? Maybe there's a better name for that. :)
Flags: needinfo?(jhofmann)
Priority: -- → P2

Updated

4 months ago
Blocks: 990185
Whiteboard: [form autofill]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

4 months ago
Assignee: nobody → schung

Updated

4 months ago
Whiteboard: [form autofill] → [form autofill:MVP]

Comment 6

4 months ago
mozreview-review
Comment on attachment 8890799 [details]
Bug 1377006 - Part 1: Add disable highlight option in main action button.

https://reviewboard.mozilla.org/r/162004/#review167750

I think this works for me, but what do you think we should do in this case:

https://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/toolkit/themes/windows/global/notification.css#85

On Windows, the buttons get swapped based on the [default] field, because Windows defines the default action to be on the left (unlike OSX where it's on the right, for LTR languages).

Should we still swap them in your case to stay consistent with the general doorhanger behavior?

Comment 7

4 months ago
mozreview-review
Comment on attachment 8890799 [details]
Bug 1377006 - Part 1: Add disable highlight option in main action button.

https://reviewboard.mozilla.org/r/162004/#review167756

Actually, canceling review since you'll need to update that rule anyway, based on what we decide to do.
Attachment #8890799 - Flags: review?(jhofmann)
(Assignee)

Comment 8

4 months ago
(In reply to Johann Hofmann [:johannh] from comment #6)
> Comment on attachment 8890799 [details]
> Bug 1377006 - Part 1: Add disable highlight option in main action button.
> 
> https://reviewboard.mozilla.org/r/162004/#review167750
> 
> I think this works for me, but what do you think we should do in this case:
> 
> https://searchfox.org/mozilla-central/rev/
> 09c065976fd4f18d4ad764d7cb4bbc684bf56714/toolkit/themes/windows/global/
> notification.css#85
> 
> On Windows, the buttons get swapped based on the [default] field, because
> Windows defines the default action to be on the left (unlike OSX where it's
> on the right, for LTR languages).
> 
> Should we still swap them in your case to stay consistent with the general
> doorhanger behavior?

Thanks for the reminder, I guess it must bring some issues in multiple buttons case base on your description. But it also reminds me that I only need to apply this non-highlight style to single button case(main action only), because designer doesn't want users pays too much attention on the button. I believe we'll still need some differentiation for multiple button case, so I have 2 solutions in my mind for now:

1. Having another property for highlight styling only.
2. Only apply this disableHighlight option if there's only one main action in the doorhanger.

Please let me know if you have any other idea, thanks!
Flags: needinfo?(jhofmann)
(In reply to Steve Chung [:steveck] from comment #8)
> (In reply to Johann Hofmann [:johannh] from comment #6)
> > Comment on attachment 8890799 [details]
> > Bug 1377006 - Part 1: Add disable highlight option in main action button.
> > 
> > https://reviewboard.mozilla.org/r/162004/#review167750
> > 
> > I think this works for me, but what do you think we should do in this case:
> > 
> > https://searchfox.org/mozilla-central/rev/
> > 09c065976fd4f18d4ad764d7cb4bbc684bf56714/toolkit/themes/windows/global/
> > notification.css#85
> > 
> > On Windows, the buttons get swapped based on the [default] field, because
> > Windows defines the default action to be on the left (unlike OSX where it's
> > on the right, for LTR languages).
> > 
> > Should we still swap them in your case to stay consistent with the general
> > doorhanger behavior?
> 
> Thanks for the reminder, I guess it must bring some issues in multiple
> buttons case base on your description. But it also reminds me that I only
> need to apply this non-highlight style to single button case(main action
> only), because designer doesn't want users pays too much attention on the
> button. I believe we'll still need some differentiation for multiple button
> case, so I have 2 solutions in my mind for now:
> 
> 1. Having another property for highlight styling only.
> 2. Only apply this disableHighlight option if there's only one main action
> in the doorhanger.
> 
> Please let me know if you have any other idea, thanks!

I think the first solution (adding something like a no-highlight class that is decoupled from the default option) sounds best to me then.
Flags: needinfo?(jhofmann)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 12

2 months ago
mozreview-review
Comment on attachment 8890799 [details]
Bug 1377006 - Part 1: Add disable highlight option in main action button.

https://reviewboard.mozilla.org/r/162004/#review182548

r=me with the CSS rule fixed. Thanks!

::: toolkit/themes/shared/popupnotification.inc.css:90
(Diff revision 2)
> -.popup-notification-button[default]:not([disabled]) {
> +.popup-notification-button[default][highlight="true"]:not([disabled]) {
>    background-color: #0996f8;
>    color: white;
>  }
>  
> -.popup-notification-button[default]:hover:not([disabled]) {
> +.popup-notification-button[defaul][highlight="true"]:hover:not([disabled]) {

You're missing a "t" in [default] here.
Attachment #8890799 - Flags: review?(jhofmann) → review+

Comment 13

2 months ago
mozreview-review
Comment on attachment 8890800 [details]
Bug 1377006 - Part 2: Add disableHighlight option in formautofill doorhanger.

https://reviewboard.mozilla.org/r/162006/#review182606
Attachment #8890800 - Flags: review?(lchang) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8890800 - Flags: review?(MattN+bmo)
(Assignee)

Comment 16

2 months ago
Hi MattN, I've rebased the patch and it's only 1 more new option for the first time autofill doorhanger and Luke has already done the first round review. It might be better for requesting your final review, thanks.

Updated

2 months ago
status-firefox56: --- → affected
status-firefox57: --- → affected
Comment on attachment 8890800 [details]
Bug 1377006 - Part 2: Add disableHighlight option in formautofill doorhanger.

https://reviewboard.mozilla.org/r/162006/#review182618
Attachment #8890800 - Flags: review?(MattN+bmo) → review+
(Assignee)

Comment 18

2 months ago
Comment on attachment 8890799 [details]
Bug 1377006 - Part 1: Add disable highlight option in main action button.

Approval Request Comment
[Feature/Bug causing the regression]: It's an UI polish for form autofill popup notification(bug 1303510).
[User impact if declined]: The original doorhanger style could not meet UX's requirement.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]:
1. Launch firefox build without using any form autofill feature before(or open about:config and set the "extensions.formautofill.firstTimeUse" flag to true)
2. Fill a form and submit data.
3. There should be a popup notification displayed and the button should be black text on grey bg instead of white text on blue bg.
[List of other uplifts needed for the feature/fix]: N/A
[Is the change risky?]: Low
[Why is the change risky/not risky?]: It only affects doorhanger style and there's only one doorhanger will apply this style via setting popupNotification options.
[String changes made/needed]: N/A
Attachment #8890799 - Flags: approval-mozilla-beta?
(Assignee)

Comment 19

2 months ago
Comment on attachment 8890800 [details]
Bug 1377006 - Part 2: Add disableHighlight option in formautofill doorhanger.

Approval Request Comment
[Feature/Bug causing the regression]: It's an UI polish for form autofill popup notification(bug 1303510).
[User impact if declined]: The original doorhanger style could not meet UX's requirement.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]:
1. Launch firefox build without using any form autofill feature before(or open about:config and set the "extensions.formautofill.firstTimeUse" flag to true)
2. Fill a form and submit data.
3. There should be a popup notification displayed and the button should be black text on grey bg instead of white text on blue bg.
[List of other uplifts needed for the feature/fix]: N/A
[Is the change risky?]: Low
[Why is the change risky/not risky?]: It only affects doorhanger style and there's only one doorhanger will apply this style via setting popupNotification options.
[String changes made/needed]: N/A
Attachment #8890800 - Flags: approval-mozilla-beta?
(Assignee)

Updated

2 months ago
Keywords: checkin-needed

Comment 20

2 months ago
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/735a60b17ee4
Part 1: Add disable highlight option in main action button. r=johannh
https://hg.mozilla.org/integration/autoland/rev/5ead094a4792
Part 2: Add disableHighlight option in formautofill doorhanger. r=lchang,MattN
Keywords: checkin-needed

Comment 21

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/735a60b17ee4
https://hg.mozilla.org/mozilla-central/rev/5ead094a4792
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Comment on attachment 8890799 [details]
Bug 1377006 - Part 1: Add disable highlight option in main action button.

UI polish for form autofill. Beta56+.

Hi Vance,
Can we have a QA to verify this?
Flags: needinfo?(vchen)
Attachment #8890799 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

2 months ago
Attachment #8890800 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 23

2 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/8dcbff75d8f1
https://hg.mozilla.org/releases/mozilla-beta/rev/47dcb670f8ca
status-firefox56: affected → fixed
In reply to Gerry Chang [:gchang] from comment #22)
> Can we have a QA to verify this?
Adding Gaby in the loop, she will make sure this bug gets verified.
Flags: qe-verify+
Flags: needinfo?(vchen)
Flags: needinfo?(gasofie)
QA Contact: gasofie

Comment 25

2 months ago
Verified as fixed using latest nightly 57.0a1 and 56.0b11 on MacOS  10.13, Win 10x64 and Ubuntu 14.04.
Flags: needinfo?(gasofie)

Updated

2 months ago
status-firefox56: fixed → verified
status-firefox57: fixed → verified

Updated

2 months ago
Depends on: 1399172
Created attachment 8907142 [details]
brokenbuttons.PNG

This breaks the button styling for the update notifications. See attached.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1399478

Comment 27

2 months ago
(In reply to Doug Thayer [:dthayer] from comment #26)
> Created attachment 8907142 [details]
> brokenbuttons.PNG
> 
> This breaks the button styling for the update notifications. See attached.

You need to add a border to one of the buttons in this case:

  border-left: 1px solid var(--panel-separator-color);
(In reply to Mike Kaply [:mkaply] from comment #27)
> (In reply to Doug Thayer [:dthayer] from comment #26)
> > Created attachment 8907142 [details]
> > brokenbuttons.PNG
> > 
> > This breaks the button styling for the update notifications. See attached.
> 
> You need to add a border to one of the buttons in this case:
> 
>   border-left: 1px solid var(--panel-separator-color);

We fixed it in bug 1399172.

Comment 29

2 months ago
Sorry, I thought a different problem was being reported. I'm trying to use disableHighlight for something I'm working on, but the buttons are one gray box. How is autofill avoiding this problem?
(In reply to Mike Kaply [:mkaply] from comment #29)
> Sorry, I thought a different problem was being reported. I'm trying to use
> disableHighlight for something I'm working on, but the buttons are one gray
> box. How is autofill avoiding this problem?

IIRC, we are only showing one button so there may be a bug with multiple.
(In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking you) from comment #30)
> (In reply to Mike Kaply [:mkaply] from comment #29)
> > Sorry, I thought a different problem was being reported. I'm trying to use
> > disableHighlight for something I'm working on, but the buttons are one gray
> > box. How is autofill avoiding this problem?
> 
> IIRC, we are only showing one button so there may be a bug with multiple.

Mike, in case you want to fix this in https://searchfox.org/mozilla-central/source/toolkit/themes/shared/popupnotification.inc.css I'd gladly review a patch for that :)

Updated

2 months ago
Depends on: 1401009
You need to log in before you can comment on or make changes to this bug.