Bug 28670 - [Chromium] Multi-value select boxes cannot do non-contiguous selection
Summary: [Chromium] Multi-value select boxes cannot do non-contiguous selection
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Nobody
URL: http://code.google.com/p/chromium/iss...
Keywords:
Depends on:
Blocks:
 
Reported: 2009-08-23 13:58 PDT by Robert Sesek
Modified: 2009-08-26 06:56 PDT (History)
5 users (show)

See Also:


Attachments
Fix (1.47 KB, patch)
2009-08-23 14:31 PDT, Robert Sesek
no flags Details | Formatted Diff | Diff
Adjusted condition per comment 6 (1.51 KB, patch)
2009-08-24 05:33 PDT, Robert Sesek
mrowe: review+
mrowe: commit-queue-
Details | Formatted Diff | Diff
Using just PLATFORM(DARWIN) (1.47 KB, patch)
2009-08-24 11:56 PDT, Robert Sesek
mrowe: review-
eric: commit-queue-
Details | Formatted Diff | Diff
FINAL- Revised Changelog (1.51 KB, patch)
2009-08-24 17:03 PDT, Robert Sesek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Sesek 2009-08-23 13:58:32 PDT
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.
Comment 1 Robert Sesek 2009-08-23 14:31:00 PDT
Created attachment 38458 [details]
Fix
Comment 2 Jeremy Moskovich 2009-08-23 15:21:31 PDT
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.
Comment 3 Sam Weinig 2009-08-23 16:40:20 PDT
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?
Comment 4 Robert Sesek 2009-08-23 16:53:05 PDT
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).
Comment 5 Darin Adler 2009-08-23 22:13:23 PDT
(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.
Comment 6 Robert Sesek 2009-08-24 05:28:47 PDT
(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.
Comment 7 Robert Sesek 2009-08-24 05:33:59 PDT
Created attachment 38478 [details]
Adjusted condition per comment 6
Comment 8 Darin Adler 2009-08-24 10:47:37 PDT
(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!
Comment 9 Jeremy Moskovich 2009-08-24 11:39:19 PDT
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.
Comment 10 Robert Sesek 2009-08-24 11:56:13 PDT
Created attachment 38489 [details]
Using just PLATFORM(DARWIN)
Comment 11 Eric Seidel (no email) 2009-08-24 12:28:51 PDT
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. :)
Comment 12 Eric Seidel (no email) 2009-08-24 13:29:03 PDT
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
Comment 13 Eric Seidel (no email) 2009-08-24 13:40:07 PDT
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.
Comment 14 Mark Rowe (bdash) 2009-08-24 13:44:52 PDT
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.
Comment 15 Eric Seidel (no email) 2009-08-24 13:47:19 PDT
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
Comment 16 Mark Rowe (bdash) 2009-08-24 13:51:43 PDT
Comment on attachment 38489 [details]
Using just PLATFORM(DARWIN)

Clearing r+ since this is not correct.
Comment 17 Jeremy Moskovich 2009-08-24 13:55:09 PDT
Mark: What do you think the right #ifdef is here?
Comment 18 Darin Fisher (:fishd, Google) 2009-08-24 14:57:05 PDT
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.
Comment 19 Mark Rowe (bdash) 2009-08-24 16:51:45 PDT
(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.
Comment 20 Mark Rowe (bdash) 2009-08-24 16:53:20 PDT
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 @@
> +2009-08-23  Robert Sesek  <rsesek@chromium.org>
> +
> +        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
Comment 21 Robert Sesek 2009-08-24 17:03:58 PDT
Created attachment 38514 [details]
FINAL- Revised Changelog

Thanks, Mark. I've added a new patch with the revised Changelog.
Comment 22 Eric Seidel (no email) 2009-08-25 16:15:09 PDT
Is the final patch supposed to be marked for review?
Comment 23 Robert Sesek 2009-08-25 16:20:40 PDT
Comment on attachment 38514 [details]
FINAL- Revised Changelog

Oops, you're right. Flag set, now.
Comment 24 Eric Seidel (no email) 2009-08-26 03:17:01 PDT
Comment on attachment 38478 [details]
Adjusted condition per comment 6

obsoleting this patch since a new one has been posted.
Comment 25 Eric Seidel (no email) 2009-08-26 03:17:39 PDT
Comment on attachment 38514 [details]
FINAL- Revised Changelog

LGTM.
Comment 26 Eric Seidel (no email) 2009-08-26 06:56:23 PDT
Comment on attachment 38514 [details]
FINAL- Revised Changelog

Clearing flags on attachment: 38514

Committed r47777: <http://trac.webkit.org/changeset/47777>
Comment 27 Eric Seidel (no email) 2009-08-26 06:56:28 PDT
All reviewed patches have been landed.  Closing bug.