Closed Bug 1046732 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: DOM: Selection, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: masayuki, Assigned: TYLin)

References

Details

Attachments

(2 files, 2 obsolete files)

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).
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.
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
Attachment #8466000 - Flags: review?(ehsan)
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.
Attachment #8466000 - Flags: review?(ehsan) → review+
(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.
This test logic is similar to
_test_touch_caret_timeout_by_dragging_it_to_top_left_corner_after_timout.
Attachment #8469116 - Flags: review?(ehsan)
This test logic is similar to
_test_touch_caret_timeout_by_dragging_it_to_top_left_corner_after_timout.
Attachment #8469116 - Attachment is obsolete: true
Attachment #8469116 - Flags: review?(ehsan)
Attachment #8469117 - Flags: review?(ehsan)
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?
Attachment #8469117 - Flags: review?(ehsan) → feedback+
(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 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?
Attachment #8469117 - Flags: review?(mdas)
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
Attachment #8469117 - Flags: review?(mdas) → review+
https://hg.mozilla.org/mozilla-central/rev/c2e53a571e98
https://hg.mozilla.org/mozilla-central/rev/3572dabf9acf
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.