1046732 - [Touch Caret] TouchCaret.cpp compares WidgetEvent::message and NS_WHEEL_EVENT_START

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

The line was written by the part 3 patch of bug 924692 whose author is Phoebe Chang (but recreated by Ting-Yu Lin) and reviewers are roc and smaug.
http://hg.mozilla.org/mozilla-central/annotate/aa176fcc56b8/layout/base/TouchCaret.cpp#l573

NS_WHEEL_EVENT_START isn't a specific event message. It's just a constant value for defining wheel event messages. So, it shouldn't be compared with switch statement.

I guess that it should be NS_WHEEL_START (= NS_WHEEL_EVENT_START + 1) or NS_WHEEL_WHEEL (= NS_WHEEL_EVENT_START).

Comment 1

[Ting-Yu Lin [:TYLin] (UTC-8) (🍼 Away March 11 - June 5)]

Nakano-san,

Nice catch! Thank you for reporting this. I've print a log to confirm that the event message received is 5400. We should use NS_WHEEL_WHEEL here. I'll upload a patch to fix it.

Comment 2

[Ting-Yu Lin [:TYLin] (UTC-8) (🍼 Away March 11 - June 5)]

Attachment: Fix incorrect event message used in TouchCaret.cpp.

NS_WHEEL_EVENT_START isn't a specific event message for compairing in
the switch case. Since we would like to hide touch caret whenever
receving wheel events, all the wheel event types are added.

Try result:
https://tbpl.mozilla.org/?tree=Try&rev=7bf56f49f3b4

Comment 3

[(no longer active)]

Comment on attachment 8466000 [details] [diff] [review]
Fix incorrect event message used in TouchCaret.cpp.

Review of attachment 8466000 [details] [diff] [review]:
-----------------------------------------------------------------

Please add a test for this stuff.  r=me with that.

Comment 4

[Ting-Yu Lin [:TYLin] (UTC-8) (🍼 Away March 11 - June 5)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #3)
> Comment on attachment 8466000 [details] [diff] [review]
> Fix incorrect event message used in TouchCaret.cpp.
> 
> Review of attachment 8466000 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please add a test for this stuff.  r=me with that.

Sure. I'll add a test in a separate patch later.

Comment 5

[Ting-Yu Lin [:TYLin] (UTC-8) (🍼 Away March 11 - June 5)]

Attachment: part 1 - Fix incorrect event message used in TouchCaret.cpp. (v2, carry r+)

Rebase and add r=ehsan.

Comment 6

[Ting-Yu Lin [:TYLin] (UTC-8) (🍼 Away March 11 - June 5)]

Attachment: part 2 - Test touch caret will be hidden by wheel event. (v1)

This test logic is similar to
_test_touch_caret_timeout_by_dragging_it_to_top_left_corner_after_timout.

Comment 7

[Ting-Yu Lin [:TYLin] (UTC-8) (🍼 Away March 11 - June 5)]

Attachment: part 2 - Test touch caret will be hidden by wheel event. (v2)

This test logic is similar to
_test_touch_caret_timeout_by_dragging_it_to_top_left_corner_after_timout.

Comment 8

[Ting-Yu Lin [:TYLin] (UTC-8) (🍼 Away March 11 - June 5)]

Try result:
https://tbpl.mozilla.org/?tree=Try&rev=010ee18f9798

Comment 9

[(no longer active)]

Comment on attachment 8469117 [details] [diff] [review]
part 2 - Test touch caret will be hidden by wheel event. (v2)

Review of attachment 8469117 [details] [diff] [review]:
-----------------------------------------------------------------

I don't know this framework well enough to be able to review this, but the test looks good from my limited understanding.  I would however test the rest of the event types while you're here as well.

Also, is there any reason why you didn't write a simple mochitest?

Comment 10

[Ting-Yu Lin [:TYLin] (UTC-8) (🍼 Away March 11 - June 5)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #9)
> Comment on attachment 8469117 [details] [diff] [review]
> part 2 - Test touch caret will be hidden by wheel event. (v2)
> 
> Review of attachment 8469117 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't know this framework well enough to be able to review this, but the
> test looks good from my limited understanding.  I would however test the
> rest of the event types while you're here as well.
> 
> Also, is there any reason why you didn't write a simple mochitest?

More testing is always welcome. Thanks for your help!

We had a wip patch written in marionette when developing touch caret. Adding tests for touch caret was my first assigned task in Mozilla, and I knew nothing about mochitest nor marionette. Since I'm more comfortable in Python then in Javascript, I stick to marionette.

Comment 11

[Ting-Yu Lin [:TYLin] (UTC-8) (🍼 Away March 11 - June 5)]

Comment on attachment 8469117 [details] [diff] [review]
part 2 - Test touch caret will be hidden by wheel event. (v2)

Review of attachment 8469117 [details] [diff] [review]:
-----------------------------------------------------------------

Malini, would you help review this?

Comment 12

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 8469117 [details] [diff] [review]
part 2 - Test touch caret will be hidden by wheel event. (v2)

Review of attachment 8469117 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm

Comment 13

[Ting-Yu Lin [:TYLin] (UTC-8) (🍼 Away March 11 - June 5)]

Malini, thanks!

Try result:
https://tbpl.mozilla.org/?tree=Try&rev=010ee18f9798

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c2e53a571e98
https://hg.mozilla.org/integration/mozilla-inbound/rev/3572dabf9acf

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c2e53a571e98
https://hg.mozilla.org/mozilla-central/rev/3572dabf9acf