[Doorhanger] Alternative main action button styling for form autofill

VERIFIED FIXED in Firefox 56

Status

()

defect
P2
normal
VERIFIED FIXED
2 years ago
2 years 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])

Attachments

(3 attachments)

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
Flags: needinfo?(paolo.mozmail)
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)
(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
Blocks: 990185
Whiteboard: [form autofill]
Assignee: nobody → schung
Whiteboard: [form autofill] → [form autofill:MVP]
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 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)
(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 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 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+
Attachment #8890800 - Flags: review?(MattN+bmo)
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.
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+
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?
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?
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/735a60b17ee4
https://hg.mozilla.org/mozilla-central/rev/5ead094a4792
Status: NEW → RESOLVED
Closed: 2 years ago
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+
Attachment #8890800 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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
Verified as fixed using latest nightly 57.0a1 and 56.0b11 on MacOS  10.13, Win 10x64 and Ubuntu 14.04.
Flags: needinfo?(gasofie)
Depends on: 1399172
Posted image brokenbuttons.PNG
This breaks the button styling for the update notifications. See attached.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
(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.
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 :)
Depends on: 1401009
You need to log in before you can comment on or make changes to this bug.