Bug 28670 – [Chromium] Multi-value select boxes cannot do non-contiguous selection

• Component: Forms
• Status: RESOLVED FIXED

[Robert Sesek]

On Chromium Mac, you cannot perform non-contiguous selection on a multi-value list box in the normal Mac way. On the Mac, you do this kind of selection by holding down the Command key; on Windows, you do this with the Control key. In SelectElement.cpp, the #if guard that determines which of these keys to respect uses PLATFORM(MAC), which is always false for Chromium. The fix is to use PLATFORM(DARWIN). Patch coming shortly.

[Robert Sesek]

Created attachment 38458 [details]
Fix

[Jeremy Moskovich]

Comment on attachment 38458 [details]
Fix

WebKit convention is to cc relevant parties and not to assign specific reviewers, thus I'm moving Darin to the cc list.

[Sam Weinig]

This does not seem like a Darwin level idea, it is very specific to the Mac.  Is this how we have been solving other cases similar to this?

[Robert Sesek]

We use at the top of the file:

#if PLATFORM(MAC) || (PLATFORM(CHROMIUM) && PLATFORM(DARWIN))

I spoke to Jeremy about hit and he thought we could reduce it to just PLATFORM(DARWIN).

[Darin Adler]

(In reply to comment #4)
> We use at the top of the file:
> 
> #if PLATFORM(MAC) || (PLATFORM(CHROMIUM) && PLATFORM(DARWIN))
> 
> I spoke to Jeremy about hit and he thought we could reduce it to just
> PLATFORM(DARWIN).

Mechanically that's OK, but it's not exactly the right concept. PLATFORM(DARWIN) is really supposed to be an OS-level switch like PLATFORM(WINOS) that tells you what low level OS is on the target machine. PLATFORM(DARWIN) means Apple's Unix variant with things like frameworks and certain libraries expected to be installed.

This seems to be more about which style of user interface we want. Chromium creates a non-Mac platform that wants to follow Mac user interface style because while it's virtual it is on a Mac.

To make that clear I think we'll probably need a different macro in the long run. Short run, we might be OK saying PLATFORM(DARWIN) here unless there's some other port running with PLATFORM(DARWIN) defined that doesn't want this behavior. For example, it's not too farfetched to imagine trouble if you were compiling for QT or GTK on a Mac.

[Robert Sesek]

(In reply to comment #5)
> Mechanically that's OK, but it's not exactly the right concept.
> PLATFORM(DARWIN) is really supposed to be an OS-level switch like
> PLATFORM(WINOS) that tells you what low level OS is on the target machine.
> PLATFORM(DARWIN) means Apple's Unix variant with things like frameworks and
> certain libraries expected to be installed.
> 
> This seems to be more about which style of user interface we want. Chromium
> creates a non-Mac platform that wants to follow Mac user interface style
> because while it's virtual it is on a Mac.
> 
> To make that clear I think we'll probably need a different macro in the long
> run. Short run, we might be OK saying PLATFORM(DARWIN) here unless there's some
> other port running with PLATFORM(DARWIN) defined that doesn't want this
> behavior. For example, it's not too farfetched to imagine trouble if you were
> compiling for QT or GTK on a Mac.

Then to be on the safe side, I think the answer is to use, like we do earlier in the same file, PLATFORM(MAC) || (PLATFORM(CHROMIUM) && PLATFORM(DARWIN)). From what I've discovered on mailing lists, PLATFORM(MAC) is used pretty much for Safari on OS X. Perhaps the right answer long-term is to make a PLATFORM(SAFARI) so that PLATFORM(MAC) can be used in instances like this.

[Robert Sesek]

Created attachment 38478 [details]
Adjusted condition per comment 6

[Darin Adler]

(In reply to comment #6)
> From what I've discovered on mailing lists, PLATFORM(MAC) is used pretty much
> for Safari on OS X. Perhaps the right answer long-term is to make a
> PLATFORM(SAFARI) so that PLATFORM(MAC) can be used in instances like this.

This is a great oversimplification and would not be correct. For example, Safari works on Windows. And tons of Mac OS X applications use the PLATFORM(MAC) version of WebKit, not just Safari.

PLATFORM(MAC) is about having WebKit on top of Mac OS X frameworks like AppKit. The Chromium port is peculiar because it's WebKit on Mac OS X, but striving to not be on top of something like AppKit because of the particular architecture of the Chrome browser, and instead of being on top of the OS frameworks, it's on top a special Chromium layer.

We do have a plan to fix the platform macros and make various more specific names, which should solve this problem. Maciej Stachowiak sent mail to webkit-dev a couple months ago about it. But PLATFORM(SAFARI) would be terrible and a step in the wrong direction!

[Jeremy Moskovich]

Robert:
The discussion of the right macros to use for cases like this seems to come up every time we need some code to be used on both Chrome/OS X & Safari since the current situation is sub-optimal.

I think in the long term we need to fix this but I think the discussion of what that solution is belongs in a separate bug or on webkit-dev.

Per-Darin's comment could you resubmit your first patch and use PLATFORM(DARWIN).

Sam:
Up till now we've resolved these issues by changing these to PLATFORM(DARWIN).

Darin & Sam: Thanks for your comments, I don't want to sound like we don't care about doing the right thing for this, we most certain do want a good long term solution here!  But as mentioned above, I think we should discuss this in another scope.

[Robert Sesek]

Created attachment 38489 [details]
Using just PLATFORM(DARWIN)

[Eric Seidel (no email)]

Comment on attachment 38489 [details]
Using just PLATFORM(DARWIN)

I think this change looks fine.  However some might disagree with me, given how contentious and mis-understood the PLATFORM #defines have been in the pass. I really hope that Maciej delivers his grand re-write soon to end all this needless confusion. :)

[Eric Seidel (no email)]

Comment on attachment 38489 [details]
Using just PLATFORM(DARWIN)

Rejecting patch 38489 from commit-queue.  This patch will require manual commit.

Failed to run "['git', 'svn', 'dcommit']"  exit_code: 1  cwd: None

[Eric Seidel (no email)]

Comment on attachment 38489 [details]
Using just PLATFORM(DARWIN)

commit-queue bug.  checkin occured to the ChangeLog file while the commit-queue was processing this.

[Mark Rowe (bdash)]

PLATFORM(DARWIN) really isn't the right macro to be using here.  The desired is specific to the behavior of the Mac OS X interface.  It may not make sense for an X11 application on Mac OS X, or directly on top of Darwin.

[Eric Seidel (no email)]

Comment on attachment 38489 [details]
Using just PLATFORM(DARWIN)

Rejecting patch 38489 from commit-queue.  This patch will require manual commit.

Failed to run "['git', 'svn', 'dcommit']"  exit_code: 1  cwd: None

[Mark Rowe (bdash)]

Comment on attachment 38489 [details]
Using just PLATFORM(DARWIN)

Clearing r+ since this is not correct.

[Jeremy Moskovich]

Mark: What do you think the right #ifdef is here?

[Darin Fisher (:fishd, Google)]

Here is what we have been using thus far:

  #if PLATFORM(MAC) || (PLATFORM(CHROMIUM) && PLATFORM(DARWIN))

If that is not correct, we should discuss it separately, and come up with a better solution to apply globally.

[Mark Rowe (bdash)]

(In reply to comment #18)
> Here is what we have been using thus far:
> 
>   #if PLATFORM(MAC) || (PLATFORM(CHROMIUM) && PLATFORM(DARWIN))
> 
> If that is not correct, we should discuss it separately, and come up with a
> better solution to apply globally.

While not perfect, this seems more reasonable to me.

[Mark Rowe (bdash)]

Comment on attachment 38478 [details]
Adjusted condition per comment 6

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 47695)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,20 @@
> [phone]  Robert Sesek  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [Chromium] Multi-value select boxes cannot do non-contiguous selection on Mac
> +        https://bugs.webkit.org/show_bug.cgi?id=28670
> +
> +        Change the condition regarding the key modifier check for non-contiguous
> +        selection on a multi-value select from PLATFORM(MAC) to PLATFORM(DARWIN), so
> +        that Chromium works as expected on Mac.

This doesn't describe the change that was actually made, so it'll need to be updated. The actual code change looks fine.

r=me

[Robert Sesek]

Created attachment 38514 [details]
FINAL- Revised Changelog

Thanks, Mark. I've added a new patch with the revised Changelog.

[Eric Seidel (no email)]

Is the final patch supposed to be marked for review?

[Robert Sesek]

Comment on attachment 38514 [details]
FINAL- Revised Changelog

Oops, you're right. Flag set, now.

[Eric Seidel (no email)]

Comment on attachment 38478 [details]
Adjusted condition per comment 6

obsoleting this patch since a new one has been posted.

[Eric Seidel (no email)]

Comment on attachment 38514 [details]
FINAL- Revised Changelog

LGTM.

[Eric Seidel (no email)]

Comment on attachment 38514 [details]
FINAL- Revised Changelog

Clearing flags on attachment: 38514

Committed r47777: <http://trac.webkit.org/changeset/47777>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.