Closed
Bug 1377006
Opened 7 years ago
Closed 7 years ago
[Doorhanger] Alternative main action button styling for form autofill
Categories
(Firefox :: Site Identity, defect, P2)
Firefox
Site Identity
Tracking
()
VERIFIED
FIXED
Firefox 57
People
(Reporter: steveck, Assigned: steveck)
References
(Blocks 1 open bug)
Details
(Whiteboard: [form autofill:MVP])
Attachments
(3 files)
59 bytes,
text/x-review-board-request
|
johannh
:
review+
gchang
:
approval-mozilla-beta+
|
Details |
59 bytes,
text/x-review-board-request
|
lchang
:
review+
MattN
:
review+
gchang
:
approval-mozilla-beta+
|
Details |
6.31 KB,
image/png
|
Details |
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•7 years ago
|
Flags: needinfo?(paolo.mozmail)
Comment 1•7 years 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•7 years 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
Comment 3•7 years ago
|
||
(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)
Updated•7 years ago
|
Priority: -- → P2
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → schung
Updated•7 years ago
|
Whiteboard: [form autofill] → [form autofill:MVP]
Comment 6•7 years 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•7 years 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•7 years 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)
Comment 9•7 years ago
|
||
(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•7 years 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•7 years 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•7 years ago
|
Attachment #8890800 -
Flags: review?(MattN+bmo)
Assignee | ||
Comment 16•7 years 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•7 years ago
|
status-firefox56:
--- → affected
status-firefox57:
--- → affected
Comment 17•7 years ago
|
||
mozreview-review |
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•7 years 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•7 years 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•7 years ago
|
Keywords: checkin-needed
Comment 20•7 years 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/735a60b17ee4 https://hg.mozilla.org/mozilla-central/rev/5ead094a4792
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Comment 22•7 years ago
|
||
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•7 years ago
|
Attachment #8890800 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 23•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/8dcbff75d8f1 https://hg.mozilla.org/releases/mozilla-beta/rev/47dcb670f8ca
Comment 24•7 years ago
|
||
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•7 years 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•7 years ago
|
Comment 26•7 years ago
|
||
This breaks the button styling for the update notifications. See attached.
Updated•7 years ago
|
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Comment 27•7 years 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);
Comment 28•7 years ago
|
||
(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•7 years 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?
Comment 30•7 years ago
|
||
(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.
Comment 31•7 years ago
|
||
(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 :)
You need to log in
before you can comment on or make changes to this bug.
Description
•