Bug 87403 – Implement pointer and hover CSS media features for touch screens

• Component: CSS
• Status: RESOLVED FIXED

[Rick Byers]

Implement the new features defined here, first on Chromium:
http://dev.w3.org/csswg/css4-mediaqueries/#pointer
http://dev.w3.org/csswg/css4-mediaqueries/#hover

[Rick Byers]

Created attachment 143944 [details]
Patch

[Rick Byers]

Initial working patch.  This includes the entire contents of the patch in bug 85921 - I'll wait until that lands and resolve before requesting review of this path.

With this patch, if we're on platform that sets the TouchScreenDevice setting (currently Chromium for Windows and ChromeOS), then we can return true for the (pointer) (pointer:coarse) and (hover:0) queries.  Otherwise we behave exactly as if the media feature isn't supported at all.  Eg., I don't want to report anything for pointer:none or pointer:fine when we don't know whether or not there's a mouse attached.  I'll handle the mouse case for chromium in a follow-up CL.

[Adam Barth]

Comment on attachment 143944 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143944&action=review

> Source/WebCore/css/MediaQueryEvaluator.cpp:542
> +static PointerDeviceType getLeastCapablePrimaryPointerDeviceType(Frame* frame)

We generally dislike using "get" as a verb because it's such a weak verb.  Perhaps "leastCapablePrimaryPointerDeviceType" ?

> Source/WebCore/css/MediaQueryEvaluator.cpp:547
> +    // FIXME: Should also try to determine if we know we have a mouse.

Please use complete sentences in comments.  "We should ..."

> Source/WebCore/css/MediaQueryEvaluator.cpp:557
> +static bool hoverMediaFeatureEval(CSSValue *value, RenderStyle*, Frame* frame, MediaFeaturePrefix)

CSSValue *value => CSSValue* value

> Source/WebCore/css/MediaQueryEvaluator.cpp:559
> +    PointerDeviceType p = getLeastCapablePrimaryPointerDeviceType(frame);

p => type (Please use words for variable names, not letters.)

> Source/WebCore/css/MediaQueryEvaluator.cpp:581
> +      return p != NoPointer;

Four-space indent.

> Source/WebCore/css/MediaQueryEvaluator.cpp:590
> +    }

Bad indent.

> LayoutTests/fast/media/mq-pointer-touchscreen.html:7
> +  if (window.internals) {
> +    internals.settings.setDeviceTouchScreen(true);
> +  }

No need for { }

> LayoutTests/fast/media/mq-pointer-touchscreen.html:41
> +  <p id="a">This text should be green if some pointer is known to be available.</p>
> +  <p id="b">This text should be green if it's known that no pointers are available.</p>
> +  <p id="c">This text should be green if a touch screen is known to be available.</p>
> +  <p id="d">This text should be green if a mouse (but not a coarse pointer) is known to be available.</p>
> +  <p id="e">This text should be green if the least capable primary pointer supports hover.</p>
> +  <p id="f">This text should be green if the least capable primary pointer doesn't support hover.</p>
> +  <p id="g">This text should be green if the least capable primary pointer supports hover.</p>

Can we use getComputedStyle or something so we can use a text test rather than a render tree dump?  Render tree dumps are annoying because they're different on every platform.

[Adam Barth]

By the way, you can just upload the diff after some other patch if you like.  It's fine to upload patches that don't apply to trunk or that don't compile.  That's better than having a bunch of unrelated changes in your patch, which make it hard to review

[Adam Barth]

This patch is not Chromium specific.

[Rick Byers]

Created attachment 144388 [details]
Patch

[Rick Byers]

Comment on attachment 143944 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143944&action=review

>> Source/WebCore/css/MediaQueryEvaluator.cpp:542
>> +static PointerDeviceType getLeastCapablePrimaryPointerDeviceType(Frame* frame)
> 
> We generally dislike using "get" as a verb because it's such a weak verb.  Perhaps "leastCapablePrimaryPointerDeviceType" ?

Done

>> Source/WebCore/css/MediaQueryEvaluator.cpp:547
>> +    // FIXME: Should also try to determine if we know we have a mouse.
> 
> Please use complete sentences in comments.  "We should ..."

Done

>> Source/WebCore/css/MediaQueryEvaluator.cpp:557
>> +static bool hoverMediaFeatureEval(CSSValue *value, RenderStyle*, Frame* frame, MediaFeaturePrefix)
> 
> CSSValue *value => CSSValue* value

Done

>> Source/WebCore/css/MediaQueryEvaluator.cpp:559
>> +    PointerDeviceType p = getLeastCapablePrimaryPointerDeviceType(frame);
> 
> p => type (Please use words for variable names, not letters.)

Done

>> Source/WebCore/css/MediaQueryEvaluator.cpp:581
>> +      return p != NoPointer;
> 
> Four-space indent.

Done.

>> Source/WebCore/css/MediaQueryEvaluator.cpp:590
>> +    }
> 
> Bad indent.

Done

>> LayoutTests/fast/media/mq-pointer-touchscreen.html:7
>> +  }
> 
> No need for { }

Done

>> LayoutTests/fast/media/mq-pointer-touchscreen.html:41
>> +  <p id="g">This text should be green if the least capable primary pointer supports hover.</p>
> 
> Can we use getComputedStyle or something so we can use a text test rather than a render tree dump?  Render tree dumps are annoying because they're different on every platform.

Sure, I'll start working on that now.  I debated between the two approaches.  I'd like to avoid having one test file per hardware configuration (since there will be several more), and the resulting duplication across the files.  So once I make this a JS test, I'll also refactor to have one test for "pointer" and one test for "hover" - which each test checking all hardware configurations.  

I thought it was nicer for CSS tests to be more declarative like this (at least a lot of the existing media query tests use this style), but I didn't realize the output would be different on different platforms (presumably the size of text changes?).  In general wouldn't it be better to have some modes where DumpRenderTree omits details that are known to vary across platforms and considered irrelevant to the test (in this case all sizes)?  The general approach of declarative layout testing seems nice to me.

[Rick Byers]

(In reply to comment #4)
> By the way, you can just upload the diff after some other patch if you like.  It's fine to upload patches that don't apply to trunk or that don't compile.  That's better than having a bunch of unrelated changes in your patch, which make it hard to review

Thanks.  I looked for a baseline option to "webkit-patch upload", but of course I can just create a separate non-compiling git branch.  Done.

[Adam Barth]

> I thought it was nicer for CSS tests to be more declarative like this (at least a lot of the existing media query tests use this style), but I didn't realize the output would be different on different platforms (presumably the size of text changes?).

Yeah.

> In general wouldn't it be better to have some modes where DumpRenderTree omits details that are known to vary across platforms and considered irrelevant to the test (in this case all sizes)?  The general approach of declarative layout testing seems nice to me.

Yes, we'd like to do that, but it's somewhat tricky and no one has done it yet.  We might even be able to get pixel perfect in most cases, at least for the Chromium port, by using mock scroll bars and using some repeatable Skia text rendering.(In reply to comment #8)

> Thanks.  I looked for a baseline option to "webkit-patch upload", but of course I can just create a separate non-compiling git branch.  Done.

I usually use this option:

abarth@quadzen:~/svn/webkit$ ./Tools/Scripts/webkit-patch help upload
[...]
  -g GIT_COMMIT, --git-commit=GIT_COMMIT
                        Operate on a local commit. If a range, the commits are
                        squashed into one. <ref>.... includes the working copy
                        changes. UPSTREAM can be used for the
                        upstream/tracking branch.

[Adam Barth]

Comment on attachment 144388 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144388&action=review

> Source/WebCore/css/MediaQueryEvaluator.cpp:589
> +    return (pointer == NoPointer && str == "none")
> +        || (pointer == TouchPointer && str == "coarse")
> +        || (pointer == MousePointer && str == "fine");

To confirm, this is supposed to be case sensitive?

> LayoutTests/fast/media/mq-pointer-touchscreen.html:15
> +@media screen and (pointer:coarse) {

Can you add some tests for mixed case?

> LayoutTests/fast/media/mq-pointer-touchscreen.html:24
> +@media screen and (hover:0) {

You can add some tests for non-numeric values?

[Adam Barth]

@tabatkins: Should we be using a vendor prefix in this patch?

[Adam Barth]

I wonder if we should have an ENABLE(TOUCH_MEDIA_QUERIES) macro for this patch.  It seems like other ports won't want to ship this code if they haven't wired up the settings property.

Also, we probably should email webkit-dev to let folks know we're implementing this feature: http://www.webkit.org/coding/adding-features.html

I'm also slightly worried that we're doing the wrong thing in the common case.  In the common case, we have a mouse pointer, but we're treating that case as "unknown" currently.  If folks write content using this feature, that means that "unknown" will de facto mean "mouse".  It seems better to wire in a setting for whether the device supports mice.

[Rick Byers]

(In reply to comment #12)
> I wonder if we should have an ENABLE(TOUCH_MEDIA_QUERIES) macro for this patch.  It seems like other ports won't want to ship this code if they haven't wired up the settings property.

I was hoping this wouldn't be necessary since I've been careful to make the PointerUnknown case behave identically to the case where the media query isn't supported at all.  Let me know if you think it's required.  Do I then need to request a bot be setup that runs with this feature enabled?
 
> Also, we probably should email webkit-dev to let folks know we're implementing this feature: http://www.webkit.org/coding/adding-features.html

Sure, I'm happy to do that.  Let's answer these other two questions firs though.

> I'm also slightly worried that we're doing the wrong thing in the common case.  In the common case, we have a mouse pointer, but we're treating that case as "unknown" currently.  If folks write content using this feature, that means that "unknown" will de facto mean "mouse".  It seems better to wire in a setting for whether the device supports mice.

Note that the "unknown" case is identical to the "feature not supported" case. Having to handle the not supported case is something sites will need to do anyway to support browsers that don't yet have this feature.  Since sites will (for some time anyway) always need some fallback decision in their app, it seems better to me to leave that decision to the app then to try to apply some heuristic behavior ourselves (like encouraging ports to assume there is a mouse when we're not sure).

I agree though that I'm not 100% positive this is the right approach (this is what I was describing in my original e-mail to you).  I can't think of a concrete scenario where it might be a problem though, any suggestions?

[Rick Byers]

(In reply to comment #13)
> (In reply to comment #12)
> > I'm also slightly worried that we're doing the wrong thing in the common case.  In the common case, we have a mouse pointer, but we're treating that case as "unknown" currently.  If folks write content using this feature, that means that "unknown" will de facto mean "mouse".  It seems better to wire in a setting for whether the device supports mice.
> 
> Note that the "unknown" case is identical to the "feature not supported" case. Having to handle the not supported case is something sites will need to do anyway to support browsers that don't yet have this feature.  Since sites will (for some time anyway) always need some fallback decision in their app, it seems better to me to leave that decision to the app then to try to apply some heuristic behavior ourselves (like encouraging ports to assume there is a mouse when we're not sure).
> 
> I agree though that I'm not 100% positive this is the right approach (this is what I was describing in my original e-mail to you).  I can't think of a concrete scenario where it might be a problem though, any suggestions?

Also, just to be clear, I intend to implement the mouse case for chromium.  It's just not as urgent as touch for me right now (I really need pointer:coarse in Chrome 21).  I can split the mouse piece out to a separate bug if you like.

Note that "unknown" (i.e. both (pointer)" and (pointer:none) being false) already de facto means "mouse" - that's the web as it exists today.  So I don't think I'm changing anything in that respect.  First step is for (pointer:coarse) to mean "definitely a touch screen", then later I'll add (pointer:fine) to mean definitely a mouse but no touch screen, and (pointer:none) to mean definitely no pointer.  Sound reasonable?

If you think it's important then I'm willing to wait and do the whole thing in one big CL instead.  Let me know.  Thanks!
   Rick

[Adam Barth]

If you add an ENABLE macro, then the order of implementation is less important because each port can decide whether or not to enable the feature.  Currently, the patch doesn't have an ENABLE macro, which means that it will report "unknown" for non-Chromium ports, even if they are using touch devices.

[Rick Byers]

Created attachment 144600 [details]
Patch

[Rick Byers]

(In reply to comment #10)
> (From update of attachment 144388 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144388&action=review
> 
> > Source/WebCore/css/MediaQueryEvaluator.cpp:589
> > +    return (pointer == NoPointer && str == "none")
> > +        || (pointer == TouchPointer && str == "coarse")
> > +        || (pointer == MousePointer && str == "fine");
> 
> To confirm, this is supposed to be case sensitive?

It's case insensitive but it's normalized to lowercase by the parsing code.  I've added tests that verify this.

> > LayoutTests/fast/media/mq-pointer-touchscreen.html:15
> > +@media screen and (pointer:coarse) {
> 
> Can you add some tests for mixed case?

Done

> > LayoutTests/fast/media/mq-pointer-touchscreen.html:24
> > +@media screen and (hover:0) {
> 
> You can add some tests for non-numeric values?

Done

[Rick Byers]

(In reply to comment #15)
> If you add an ENABLE macro, then the order of implementation is less important because each port can decide whether or not to enable the feature.  Currently, the patch doesn't have an ENABLE macro, which means that it will report "unknown" for non-Chromium ports, even if they are using touch devices.

What exactly do you mean "report unknown"?  

If I add an ENABLE macro, then ports that haven't opted into this will return 'false' for all relevant queries.  Without the ENABLE macro, ports that haven't opted into supplying pointer device data return 'false' for all relevant queries - i.e. exactly the same behavior as with the macro.  So the two choices we're debating have identical behavior.

That said, if there's other reasons to use an ENABLE macro I'm fine doing that.  I just want to make sure I'm clear on the benefits it provides, and importantly what the compat implications of enabling the feature are.

[Adam Barth]

Comment on attachment 144600 [details]
Patch

Thanks for clarifying!  I see that now in the spreadsheet you gave me.  My apologies, I was confused before.

[Rick Byers]

(In reply to comment #19)
> (From update of attachment 144600 [details])
> Thanks for clarifying!  I see that now in the spreadsheet you gave me.  My apologies, I was confused before.

Cool, thanks!  I'll make that post to webkit-dev before putting this in the commit queue.

[Rick Byers]

(In reply to comment #11)
> @tabatkins: Should we be using a vendor prefix in this patch?

I spoke with Tab over IM and he said "Honestly, I'd recommend no prefix.  This is not going to be controversial."

[Rick Byers]

I've filed https://bugs.webkit.org/show_bug.cgi?id=87806 to track adding the mouse support piece, since my patch here is just about touch screens.

[WebKit Review Bot]

Comment on attachment 144600 [details]
Patch

Rejecting attachment 144600 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Review Bot]

Comment on attachment 144600 [details]
Patch

Rejecting attachment 144600 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
 of 2 hunks FAILED -- saving rejects to file Source/WebCore/testing/InternalSettings.h.rej
patching file Source/WebCore/testing/InternalSettings.idl
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/fast/media/mq-pointer-expected.txt
patching file LayoutTests/fast/media/mq-pointer.html

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Adam Barth']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/12849950

[Rick Byers]

Created attachment 144940 [details]
Merge with trunk, resolve conflicts

[WebKit Review Bot]

Comment on attachment 144940 [details]
Merge with trunk, resolve conflicts

Clearing flags on attachment: 144940

Committed r119045: <http://trac.webkit.org/changeset/119045>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.