Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2931433002:
Disable hover state change for touch event on page with viewport meta and mobile. (Closed)
DescriptionDisable hover state change for touch event on page with viewport meta and mobile.
This patch prevent hover state change for touch event on mobile.
We have 2 checks for this:
1. Event is from touch.
2. Viewport enabled (on mobile).
3. Page with viewport meta.
BUG=369044
Review-Url: https://codereview.chromium.org/2931433002
Cr-Commit-Position: refs/heads/master@{#478777}
Committed: https://chromium.googlesource.com/chromium/src/+/876bb220728d85b1c8a8fe93a0a95bafcf448cd6
Patch Set 1 #
Total comments: 2
Patch Set 2 : only change hover state #Patch Set 3 : add tests #
Total comments: 3
Patch Set 4 : add no viewport test #
Total comments: 1
Patch Set 5 : merge crrev.com/2934853002 #
Depends on Patchset: Messages
Total messages: 32 (21 generated)
chaopeng@chromium.org changed reviewers: + bokan@chromium.org
chaopeng@chromium.org changed reviewers: + bokan@chromium.org
Please take an early look at this one. https://codereview.chromium.org/2931433002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2931433002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Document.cpp:6491: element->SetActive(true); I don't know should we keep setActive here?
Please take an early look at this one. https://codereview.chromium.org/2931433002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2931433002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Document.cpp:6491: element->SetActive(true); I don't know should we keep setActive here?
This approach looks fine to me, but we need to keep :active state https://codereview.chromium.org/2931433002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2931433002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Document.cpp:6491: element->SetActive(true); On 2017/06/06 17:18:49, chaopeng wrote: > I don't know should we keep setActive here? Active makes sense in the context of touch so we can't just early out above - skip only the hover related parts.
The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Disable hover state change for touch event on mobile. This patch prevent hover state change for touch event on mobile. We have 2 checks for this: 1. Event is from touch. 2. VisualViewport::ShouldDisableDesktopWorkarounds need on mobile and has viewport meta. BUG=369044 ========== to ========== Disable hover state change for touch event on mobile. This patch prevent hover state change for touch event on mobile. We have 2 checks for this: 1. Event is from touch. 2. VisualViewport::ShouldDisableDesktopWorkarounds returns true. BUG=369044 TESTS: - TapChangeHoverStateTest.TapNotChangeHoverStateOnMobile, - TapChangeHoverStateTest.TapChangeHoverStateOnDesktop ==========
On 2017/06/06 21:07:09, bokan wrote: > This approach looks fine to me, but we need to keep :active state > > https://codereview.chromium.org/2931433002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/dom/Document.cpp (right): > > https://codereview.chromium.org/2931433002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/dom/Document.cpp:6491: element->SetActive(true); > On 2017/06/06 17:18:49, chaopeng wrote: > > I don't know should we keep setActive here? > > Active makes sense in the context of touch so we can't just early out above - > skip only the hover related parts. Updated, PTAL. Thank you.
https://codereview.chromium.org/2931433002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (left): https://codereview.chromium.org/2931433002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11658: web_view->CoreHitTestResultAt(WebPoint(175, 1)); Why this change? What was this doing before? https://codereview.chromium.org/2931433002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/2931433002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11703: if (on_mobile) { The difference isn't whether we're on mobile or not, it's whether we have a viewport meta tag with width=device-width (though viewport is only used on mobile). i.e. We can still have hover on mobile. So your test here should add a meta tag if we expect to disable hover and load a page without a meta tag (or with and a really large width) and check that hover is still activated. You'll also want to update the commit message to say this as currently it says "disable hover on mobile" which isn't actually true.
Description was changed from ========== Disable hover state change for touch event on mobile. This patch prevent hover state change for touch event on mobile. We have 2 checks for this: 1. Event is from touch. 2. VisualViewport::ShouldDisableDesktopWorkarounds returns true. BUG=369044 TESTS: - TapChangeHoverStateTest.TapNotChangeHoverStateOnMobile, - TapChangeHoverStateTest.TapChangeHoverStateOnDesktop ========== to ========== Disable hover state change for touch event on page with viewport meta and mobile. This patch prevent hover state change for touch event on mobile. We have 2 checks for this: 1. Event is from touch. 2. Viewport enabled (on mobile). 3. Page with viewport meta. BUG=369044 TESTS: - TapChangeHoverStateTest.TapNotChangeHoverStateOnMobile, - TapChangeHoverStateTest.TapChangeHoverStateOnDesktop ==========
Description was changed from ========== Disable hover state change for touch event on page with viewport meta and mobile. This patch prevent hover state change for touch event on mobile. We have 2 checks for this: 1. Event is from touch. 2. Viewport enabled (on mobile). 3. Page with viewport meta. BUG=369044 TESTS: - TapChangeHoverStateTest.TapNotChangeHoverStateOnMobile, - TapChangeHoverStateTest.TapChangeHoverStateOnDesktop ========== to ========== Disable hover state change for touch event on page with viewport meta and mobile. This patch prevent hover state change for touch event on mobile. We have 2 checks for this: 1. Event is from touch. 2. Viewport enabled (on mobile). 3. Page with viewport meta. BUG=369044 TESTS: - TapChangeHoverStateTest.TapNotChangeHoverStateOnViewportMetaAndMobile - TapChangeHoverStateTest.TapChangeHoverStateOnNoViewportMetaAndMobile - TapChangeHoverStateTest.TapChangeHoverStateOnViewportMetaAndDesktop ==========
Updated, PTAL. https://codereview.chromium.org/2931433002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (left): https://codereview.chromium.org/2931433002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11658: web_view->CoreHitTestResultAt(WebPoint(175, 1)); On 2017/06/09 16:21:09, bokan wrote: > Why this change? What was this doing before? This hit_test_result is unused var now. hit_test_result used to verify the hit test result include/exclude something.
https://codereview.chromium.org/2931433002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/2931433002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11712: ApplyViewportStyleOverride(&web_view_helper); Lets avoid this since it uses DevTools emulation so I'd rather not depend on that. Just load a different html file that has a viewport <meta> or add one dynamically.
On 2017/06/09 19:27:55, bokan wrote: > https://codereview.chromium.org/2931433002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right): > > https://codereview.chromium.org/2931433002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/web/tests/WebFrameTest.cpp:11712: > ApplyViewportStyleOverride(&web_view_helper); > Lets avoid this since it uses DevTools emulation so I'd rather not depend on > that. Just load a different html file that has a viewport <meta> or add one > dynamically. lgtm though once that's done
The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by chaopeng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org Link to the patchset: https://codereview.chromium.org/2931433002/#ps80001 (title: "merge crrev.com/2934853002")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1497296756201130,
"parent_rev": "1b00427e8d6862527b2b4c5763feb67165f905c0", "commit_rev":
"876bb220728d85b1c8a8fe93a0a95bafcf448cd6"}
Message was sent while issue was closed.
Description was changed from ========== Disable hover state change for touch event on page with viewport meta and mobile. This patch prevent hover state change for touch event on mobile. We have 2 checks for this: 1. Event is from touch. 2. Viewport enabled (on mobile). 3. Page with viewport meta. BUG=369044 TESTS: - TapChangeHoverStateTest.TapNotChangeHoverStateOnViewportMetaAndMobile - TapChangeHoverStateTest.TapChangeHoverStateOnNoViewportMetaAndMobile - TapChangeHoverStateTest.TapChangeHoverStateOnViewportMetaAndDesktop ========== to ========== Disable hover state change for touch event on page with viewport meta and mobile. This patch prevent hover state change for touch event on mobile. We have 2 checks for this: 1. Event is from touch. 2. Viewport enabled (on mobile). 3. Page with viewport meta. TESTS: Review-Url: https://codereview.chromium.org/2931433002 Cr-Commit-Position: refs/heads/master@{#478777} Committed: https://chromium.googlesource.com/chromium/src/+/876bb220728d85b1c8a8fe93a0a9... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/876bb220728d85b1c8a8fe93a0a9...
Message was sent while issue was closed.
Description was changed from ========== Disable hover state change for touch event on page with viewport meta and mobile. This patch prevent hover state change for touch event on mobile. We have 2 checks for this: 1. Event is from touch. 2. Viewport enabled (on mobile). 3. Page with viewport meta. TESTS: Review-Url: https://codereview.chromium.org/2931433002 Cr-Commit-Position: refs/heads/master@{#478777} Committed: https://chromium.googlesource.com/chromium/src/+/876bb220728d85b1c8a8fe93a0a9... ========== to ========== Disable hover state change for touch event on page with viewport meta and mobile. This patch prevent hover state change for touch event on mobile. We have 2 checks for this: 1. Event is from touch. 2. Viewport enabled (on mobile). 3. Page with viewport meta. bug: 369044 Review-Url: https://codereview.chromium.org/2931433002 Cr-Commit-Position: refs/heads/master@{#478777} Committed: https://chromium.googlesource.com/chromium/src/+/876bb220728d85b1c8a8fe93a0a9... ==========
Message was sent while issue was closed.
Description was changed from ========== Disable hover state change for touch event on page with viewport meta and mobile. This patch prevent hover state change for touch event on mobile. We have 2 checks for this: 1. Event is from touch. 2. Viewport enabled (on mobile). 3. Page with viewport meta. bug: 369044 Review-Url: https://codereview.chromium.org/2931433002 Cr-Commit-Position: refs/heads/master@{#478777} Committed: https://chromium.googlesource.com/chromium/src/+/876bb220728d85b1c8a8fe93a0a9... ========== to ========== Disable hover state change for touch event on page with viewport meta and mobile. This patch prevent hover state change for touch event on mobile. We have 2 checks for this: 1. Event is from touch. 2. Viewport enabled (on mobile). 3. Page with viewport meta. BUG=369044 Review-Url: https://codereview.chromium.org/2931433002 Cr-Commit-Position: refs/heads/master@{#478777} Committed: https://chromium.googlesource.com/chromium/src/+/876bb220728d85b1c8a8fe93a0a9... ==========
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2991253002/ by chaopeng@chromium.org. The reason for reverting is: Revert for some website break.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
