Closed Bug 841587 Opened 7 years ago Closed 7 years ago

Allow notificationbox styles to use a different style for "default" buttons

Categories

(Toolkit :: XUL Widgets, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: mbrubeck, Assigned: mbrubeck)

References

Details

Attachments

(1 file, 5 obsolete files)

Attached patch patch (obsolete) — Splinter Review
For bug 838211, we want to use different styles for different buttons in the notification bar.  This patch adds an option "className" property that callers can set in elements of the "aButtons" parameter to appendNotification.

It also adds a default "notification-button" class for all notification buttons, to aid in styling them efficiently with CSS.
Attachment #714145 - Flags: review?(dao)
Comment on attachment 714145 [details] [diff] [review]
patch

If I interpret attachment 713273 [details] correctly, the goal is to set something like a default button, i.e. the button with the action that users will want to pick in most cases. So the API should provide the ability to do exactly that rather than expecting each consumer to provide a class name. However, it also looks like the default button is always going to be the first button, so maybe no new API is needed at all.
Attachment #714145 - Flags: review?(dao) → review-
Attached patch patch v2 (obsolete) — Splinter Review
(In reply to Dão Gottwald [:dao] from comment #1)
> If I interpret attachment 713273 [details] correctly, the goal is to set
> something like a default button, i.e. the button with the action that users
> will want to pick in most cases. So the API should provide the ability to do
> exactly that rather than expecting each consumer to provide a class name.

Sounds good.  This revised patch adds an optional "isDefault" property for the button, and adds a class for all buttons as well as for default buttons.  (I think this is better than relying on the button order, both for code robustness/clarity and for CSS selector efficiency.)
Attachment #714145 - Attachment is obsolete: true
Attachment #714435 - Flags: review?(dao)
I don't think it makes much sense to require every consumer to set the isDefault flag. We should instead just use the first button. It doesn't matter for CSS selector efficiency; notification.xml can just set the notification-button-default class on the first button. If and when we have notifications that want a default button that isn't the first one, we can extend the API to support that.
Comment on attachment 714435 [details] [diff] [review]
patch v2

(In reply to Dão Gottwald [:dao] from comment #3)
> I don't think it makes much sense to require every consumer to set the
> isDefault flag. We should instead just use the first button.

We don't have consensus yet on whether the first button will always be default (or even whether there *is* always a default).

For example, the specs for both geolocation and popup blocking say the first button should be "Allow once", but shorlander told me on IRC that "Allow once" should be the default in the first case but not the second.  I'll talk this over with Asa and Stephen, then update this patch as needed.  We can put this bug on hold in the meantime.
Attachment #714435 - Flags: review?(dao)
(In reply to Matt Brubeck (:mbrubeck) from comment #4)
> Comment on attachment 714435 [details] [diff] [review]
> patch v2
> 
> (In reply to Dão Gottwald [:dao] from comment #3)
> > I don't think it makes much sense to require every consumer to set the
> > isDefault flag. We should instead just use the first button.
> 
> We don't have consensus yet on whether the first button will always be
> default

We don't need consensus that it /will always/ be the default case, as we can extend the API to support uncommon cases when we run into them. We just need consensus that we usually want the first button to be the default one, which should therefore be the default behavior without callers opting in.
We don't need consensus that this /will always/ be the case, is what I meant to say.
Summary: Allow notificationbox users to set a className on notification buttons → Allow notificationbox styles to use a different style for "default" buttons
Attached patch patch v3 (obsolete) — Splinter Review
(In reply to Dão Gottwald [:dao] from comment #5)
> We don't need consensus that it /will always/ be the default case, as we can
> extend the API to support uncommon cases when we run into them. We just need
> consensus that we usually want the first button to be the default one, which
> should therefore be the default behavior without callers opting in.

Ah, good point.  This patch treats the first button as "default" and the rest as "non-default" unless otherwise specified, so most callers will not need to pass any extra data.

Since we already have one case in Metro where the first option is *not* default, it also allows callers to explicitly set "isDefault" to true or false on any button.

The code does not enforce any constraint on the number of default buttons, leaving that up to the caller to decide.  Some notifications, like "popup blocked" in Metro, will want zero default buttons.  (If you think we should limit the number to <= 1, that does seems reasonable to me; I just couldn't think of any strong arguments for or against enforcing that here.)

Thanks for the reviews and feedback so far!
Attachment #714435 - Attachment is obsolete: true
Attachment #714570 - Flags: review?(dao)
Comment on attachment 714570 [details] [diff] [review]
patch v3

Review of attachment 714570 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/widgets/notification.xml
@@ +116,5 @@
> +                buttonElem.classList.add("notification-button");
> +
> +                // Usually the first button is the default. Callers can
> +                // optionally set "isDefault" to override this for any button.
> +                var isDefault = ("isDefault" in button) ? button.isDefault : (b == 0);

doesn't this return true for *both* the first button and the default button? I assume you want instead a var out of the loop to track which element is the default, search for it in the loop (default is the first one) and add the class out of the loop.
(In reply to Marco Bonardo [:mak] (Away Feb 22) from comment #8)
> doesn't this return true for *both* the first button and the default button?
> I assume you want instead a var out of the loop to track which element is
> the default, search for it in the loop (default is the first one) and add
> the class out of the loop.

As I said above, the code does not limit the number of default buttons -- but I'd be happy to do it as you suggest if you think that is clearer.  I assume the default class would be applied only to the first element with "isDefault: true", or else to the first button (unless it has "isDefault: false")?

Right now the only place where we actually override the default behavior is to turn it *off* for the first button, like this:
https://bugzilla.mozilla.org/attachment.cgi?id=716059&action=diff#a/browser/metro/base/content/browser.js_sec2
Personally (I'm not your reviewer here so this is just my opinion) I think it would be clearer and less error-prone to exit the loop with a single element (or null) and set the class on it, if defined.  Unless there is a clear request to have multiple default buttons, but that sounds wrong per definition, unless "default" here means something else.
I agree we don't want multiple default buttons. For a clearer API we could also (instead of the isDefault flag) add a defaultButton option -- it would be an index and could be set to -1 to support the no-default-button case.
Attached patch patch v4 (obsolete) — Splinter Review
Thanks, guys. I agree this is a nicer API.
Attachment #714570 - Attachment is obsolete: true
Attachment #714570 - Flags: review?(dao)
Attachment #716073 - Flags: review?(dao)
Ugh, I thought there would be a PopupNotifications.show style options object to hook into. I'd rather not add a separate parameter for this and go back to the isDefault flag approach.
Attached patch patch v5 (obsolete) — Splinter Review
Yeah, I think that's why I shied away from passing an index to begin with. :(  This patch goes back to the isDefault flag but implements the logic suggested by Marco above.
Attachment #716073 - Attachment is obsolete: true
Attachment #716073 - Flags: review?(dao)
Attachment #716089 - Flags: review?(dao)
Comment on attachment 716089 [details] [diff] [review]
patch v5

Review of attachment 716089 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/widgets/notification.xml
@@ +126,5 @@
> +                  defaultElem = buttonElem;
> +                  hasExplicitDefault = true;
> +                } else if (b == 0 && !("isDefault" in button && button.isDefault === false)) {
> +                  defaultElem = buttonElem;
> +                }

what about:

let isDefault = button.isDefault || (b == 0 && !("isDefault" in button));
if (!defaultElem && isDefault)
  defaultElem = buttonElem;
(In reply to Marco Bonardo [:mak] (Away Feb 22) from comment #15)
> let isDefault = button.isDefault || (b == 0 && !("isDefault" in button));
> if (!defaultElem && isDefault)
>   defaultElem = buttonElem;

If you set "isDefault: true" on the second button only, that would still set the first button as default rather than the second button.

So that would require callers to set "isDefault: false" on the first button in addition to "isDefault: true" on the actual default button.  I think we want to avoid such a requirement.
(In reply to Matt Brubeck (:mbrubeck) from comment #16)
> If you set "isDefault: true" on the second button only, that would still set
> the first button as default rather than the second button.

ugh, let me retry last time :)

// The first button is always default unless it opts-out...
let firstButtonDefault = b == 0 && button.isDefault !== false;
// Though the first button that opts-in to be default overrides it.
let buttonOverridesDefault = button.isDefault &&
                             (!defaultElem || !defaultElem.isDefault);
if (firstButtonDefault || buttonOverridesDefault)
  defaultElem = buttonElem;
(In reply to Marco Bonardo [:mak] (Away Feb 22) from comment #17)
> // The first button is always default unless it opts-out...
> let firstButtonDefault = b == 0 && button.isDefault !== false;

Note that this will log a strict mode warning if "button" has no "isDefault" property.  I don't know how much we care about that.

> // Though the first button that opts-in to be default overrides it.
> let buttonOverridesDefault = button.isDefault &&
>                              (!defaultElem || !defaultElem.isDefault);

This won't quite work as-is, because the "isDefault" property is set on the passed-in object ("button"), not the element ("buttonElem").  You would need to store both the object and the element, or copy the property onto the element or something.
Comment on attachment 716089 [details] [diff] [review]
patch v5

>+                if ("isDefault" in button && button.isDefault && !hasExplicitDefault) {
>+                  defaultElem = buttonElem;
>+                  hasExplicitDefault = true;
>+                } else if (b == 0 && !("isDefault" in button && button.isDefault === false)) {
>+                  defaultElem = buttonElem;
>+                }

Can be simplified like this:

if (button.isDefault ||
    b == 0 && !("isDefault" in button))
  defaultElem = buttonElem;

Only difference is that if isDefault is erroneously specified multiple times, we honor the last occurrence rather than the first one.
Priority: -- → P1
Attached patch patch v6Splinter Review
Attachment #716089 - Attachment is obsolete: true
Attachment #716089 - Flags: review?(dao)
Attachment #717405 - Flags: review?(dao)
Attachment #717405 - Flags: review?(dao) → review+
Assuming there is one, what's your justification for inventing a new class rather than using default="true" styling?
(In reply to neil@parkwaycc.co.uk from comment #22)
> Assuming there is one, what's your justification for inventing a new class
> rather than using default="true" styling?

default="true" indicates that you can hit Enter to activate the button even when it's not focused. That's not how notification bars work.
So what will a default notification button look like?
https://hg.mozilla.org/mozilla-central/rev/5c3d85cb3cef
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
(In reply to neil@parkwaycc.co.uk from comment #24)
> So what will a default notification button look like?

See attachment 713273 [details] for Metro-specific mockups. I don't think there are plans for other platforms.
Depends on: 926859
No longer depends on: 926859
You need to log in before you can comment on or make changes to this bug.