929496 - Make GTK color-picker dialog non-modal again, without allowing the same <input type="color"> element to spawn multiple dialogs

Status: VERIFIED FIXED

(Core :: Widget: Gtk, defect)

Description

[Arnaud Bienner]

As discussed in bug 917917, the Gtk color picker (used by input type color) behavior can be improved.
Reminder: in the first place, the dialog window created wasn't modal, but this allows user to open (too) many color pickers. We fixed this in bug 917917 by making the picker modal. But, as we agree, this was a quick fix as there is no real reason for making it modal.

Solutions proposed were:
(1) Allow at most 1 dialog
(2) Allow at most 1 dialog per Firefox window
(3) Allow at most 1 dialog per color-input button. (so, multiple dialogs can be displayed simultaneously if you have multiple buttons)
(4) Keep the current behavior (modal windows).

Modal windows prevent user to select text, scroll window etc. and I see no real reasons why we should prevent this.
Even if we restrict the number of color pickers opened at a time, having to many color pickers might be confusing for user (will not be obvious to which input a color picker is related), so I'm not sure (3) is a good solution. (2) sounds better, but (1) is even simpler.
IMHO (1) would be the best solution.
About (1): I think the picker should be closed a new brand new one should be opened instead of re-using the opened one, because it's more meaningful IMO, mostly because the new color picker will be instantiated with the input's color.

For information, IIRC, Windows's color picker is modal, and Mac OS one allows one instance to be opened at a time. For both, it's a system limitation.

Comment 1

[Arnaud Bienner]

Adding karlt and dholbert to the cc list as they were part of the discussion about this in bugs 875753 and 917917.

Comment 2

[Daniel Holbert [:dholbert]]

[Adjusted summary to better-approximate the work items that would be covered by this bug. Note that if we were to go for option (4), then this bug is just WONTFIX and no patches are needed.]

Comment 3

[Arnaud Bienner]

Note that due to the changes in bug 946479, if we make the color picker popup non-modal now, we will end up with behavior (3), as HTMLInputElement doesn't allow more than one picker to be opened at a time for an input element now.

(3) sounds like a good behavior for me, as it's more flexible (why should we prevent a user to select two colors at a time if he really wants to?) even if I agree it might be confusing to have multiples color picker popup opened at a time (even if they are unrelated).

If we go for (1) we will probably do something similar to what we did for Windows in bug 944737 (we will be consistent, but we did this because of system limitation which we don't have on Gtk, not because of UXD concerns).

I'm not sure how we can do (2), but I guess as |nsIDOMWindow *parent| is passed through nsColorPicker::Init, we can use this information (stored in a static map or something similar) to know if a color picker is already opened for this Windows.

And, as we said, going for behavior (4) means WONFIX.

Daniel, what do you think might the best solution? Do you think we should involve someone else? (if yes, do you know who can help on this kind of UXD questions?)

Comment 4

[Daniel Holbert [:dholbert]]

I'm kind of neutral, but (3) sounds reasonable enough to me, and based on your comment, it sounds like it's a trivial change (just reverting bug 917917, right?). So if it ends up being annoying / confusing, we can always back this out.

(I think UX feedback is more relevant for visual design type stuff - not really necessary here.)

Comment 5

[Daniel Holbert [:dholbert]]

However, you probably should make sure karl isn't opposed, since this is a Widget|GTK change, and he's a Widget|GTK peer. (Per bug 917917 comment 12, it sounded like he was interested in the idea of (3) but expressed some concern.)

Looks like he's on vacation until the 28th; if you want to land this sooner, roc (the module owner) could review as well.

Comment 6

[Arnaud Bienner]

Ah yes, you're right, I forgot Karl's comment about this.

The problem with the solution he proposed is the same as what you said in bug 944737 comment 10: if we allow only one color picker to be opened at a time, closing an already opened color picker will result in a dataloss issue.
I would like to have his point of view about this.

This doesn't sound like an urgent thing to me, so I'm fine to wait for him to be back.

Comment 7

[Karl Tomlinson (:karlt)]

I'm OK with (3).  IIUC the only way to tell multiple pickers apart would be for the user to remember where the pickers opened and keep track of the windows, or if the initial colors are different.  Things may get harder if an input ends up in a background tab, or scrolled offscreen etc, but (3) gives more power to the user and Linux/GTK users should be able to cope.

Comment 8

[Arnaud Bienner]

Actually color pickers popup are "associated" to the window from where they have been opened. i.e. if the window is reduced, the color picker popup is reduced as well; and if a color picker behind is clicked/selected, the window it's associated to is pop to front.
This should make things less confusing in these cases.

For tabs, it's more difficult to know to which tab belongs a particular color picker, as you said.

But as we agree on (3), let's go for it: depending on the feedback we might receive, we will change the behavior if needed.

Comment 9

[Arnaud Bienner]

Attachment: Make GTK color-picker dialog non-modal again

Straight revert of what was done in bug 917917.

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8362710 [details] [diff] [review]
Make GTK color-picker dialog non-modal again

># Parent  61fd0f987cf2e037ff209ce9e48a30a545dd4bf7
>Bug 929496 - Make GTK color-picker dialog non-modal again, now we allow only one color to be opened at a time for a input element (since bug bug 946479). This reverts changes made in bug 917917

I think you mean "only one picker" (not "only one color"). Also, if you like, I think you could end the commit message after that; everything after that is probably too much information for a commit message.*

Also, I'm not sure I've reviewed GTK widget code before, but given that this is a trivial backout to a previously known-good state of this file, and given Karl's comment 7, I'm fine signing off on this. r=me

(*If you want to add extra information to a commit message, beyond the short one-line message, you can do it in a second line, and then it'll be there in the changeset but hidden in most concise views like TBPL and 'hg log')

Comment 11

[Arnaud Bienner]

Attachment: Make GTK color-picker dialog non-modal again: v2

New patch with commit message split and corrected.
Marking it as r+ directly, as Daniel reviewed the previous one.
As said, as it's a small and trivial change, I don't think we need to bother someone else to review this too.

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 8363333 [details] [diff] [review]
Make GTK color-picker dialog non-modal again: v2

>Bug 929496 - Make GTK color-picker dialog non-modal again. r=dholbert
>Not needed anymore now we allow only one color picker to be opened at a time for a input element (since bug bug 946479). This reverts changes made in bug 917917

The second line ("Not needed anymore") is a bit unclear - I'd word it as:
  This reverts changes made in bug 917917, which are not needed anymore [etc]

Comment 13

[Arnaud Bienner]

Attachment: Make GTK color-picker dialog non-modal again: v3

OK, I've updated the commit message.

Comment 14

[Daniel Holbert [:dholbert]]

Thanks! Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/0d54eaf336e1

Comment 15

[Daniel Holbert [:dholbert]]

--> tweaking bug summary a bit (since we aren't actually limiting the number of dialogs that can be open at a time)

Comment 16

[Tim Nguyen :ntim]

Solution (3) seems quite user unfriendly, since people might be confused (which color dialog corresponds to the color picker)

Comment 17

[Daniel Holbert [:dholbert]]

See comment 3, where Arnaud already mentioned that but pointed out that if someone *really* wants to open two dialogs (perhaps to decide on two colors that go nicely together, with two side-by-side pickers), it's a bit presumptuous of us to prevent them.

IMHO, the usefulness of that choosing-complementary-colors scenario, combined with the unlikeliness of anyone actually accidentally running into this hypothetical problem in the real world (and the unlikelihood of it causing them serious trouble) means we should tip in favor of being more permissive.

Moreover, if a website wants to have several <input type="color"> elements and wants to ensure that only one can be activated at a time, they should be able to enforce that themselves with something like
 data:text/html,<input type="color" onclick="return false">
(with a bit of extra logic in the onclick handler, to make it return false IFF a picker has been activated, or something like that).

Comment 18

[Arnaud Bienner]

This has been merged on central, and as so, this bug can be closed now, right?

Comment 19

[Daniel Holbert [:dholbert]]

Yup; the person who merged it must've forgotten to update this bug.

The central cset was: https://hg.mozilla.org/mozilla-central/rev/0d54eaf336e1

Comment 20

[Manuela Muntean [Away]]

Verified as fixed with Firefox 29 beta 1 on Ubuntu 13.10 x86: GTK color-picker dialog is non-modal again.

Demo pages used to test:

http://hg.mozilla.org/mozilla-central/raw-file/tip/layout/reftests/forms/input/color/input-color-1.html

https://bug975468.bugzilla.mozilla.org/attachment.cgi?id=8379839