Closed
Bug 987078
Opened 10 years ago
Closed 10 years ago
If pointerType is "mouse" and buttons > 0, then pressure must be 0.5
Categories
(Core :: Widget, defect)
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: alessarik, Assigned: alessarik)
References
Details
Attachments
(1 file, 3 obsolete files)
1.80 KB,
patch
|
smaug
:
review+
oleg.romashin
:
feedback+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release) Build ID: 20131205075310 Expected results: Specification: https://dvcs.w3.org/hg/pointerevents/raw-file/tip/pointerEvents.html Pressure: For hardware that does not support pressure, including but not limited to mouse, the value MUST be 0.5 when in the active buttons state and 0 otherwise.
Assignee | ||
Comment 1•10 years ago
|
||
+ add small changes of pressure Desktop FireFox need support specification of Pointer Events
Attachment #8395610 -
Flags: review?(mbrubeck)
Attachment #8395610 -
Flags: review?(mak77)
Attachment #8395610 -
Flags: review?(gavin.sharp)
Attachment #8395610 -
Flags: review?(bugs)
Attachment #8395610 -
Flags: feedback?(oleg.romashin)
Attachment #8395610 -
Flags: feedback?(nicklebedev37)
Comment 2•10 years ago
|
||
Comment on attachment 8395610 [details] [diff] [review] not_null_pressure_ver1.diff We need to make this work consistency on all the platforms. So I think this should be integrated somehow to WidgetMouseEvent, so that if .buttons is non-zero, event.pressure gets automatically set to 0.5f. So, perhaps make .pressure a private member variable and add SetButtons(flag) and GetButtons(); methods. SetButtons() would then update .pressure if the event is trusted. We don't want to explicitly set event.pressure = 0.5f in all the widget implementations. That would be error prone.
Attachment #8395610 -
Flags: review?(mbrubeck)
Attachment #8395610 -
Flags: review?(mak77)
Attachment #8395610 -
Flags: review?(gavin.sharp)
Attachment #8395610 -
Flags: review?(bugs)
Attachment #8395610 -
Flags: review-
Updated•10 years ago
|
Component: Untriaged → Widget
Product: Firefox → Core
Assignee | ||
Comment 3•10 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #2) > So, perhaps make .pressure a private member variable and > add SetButtons(flag) and GetButtons(); methods. > SetButtons() would then update .pressure if the event is trusted. I think we cannot make .pressure a private member variable, because another widget should have access to set this value.
Attachment #8395610 -
Attachment is obsolete: true
Attachment #8395610 -
Flags: feedback?(oleg.romashin)
Attachment #8395610 -
Flags: feedback?(nicklebedev37)
Attachment #8396202 -
Flags: review?(vladimir)
Attachment #8396202 -
Flags: review?(bugs)
Attachment #8396202 -
Flags: feedback?(oleg.romashin)
Attachment #8396202 -
Flags: feedback?(nicklebedev37)
Comment on attachment 8396202 [details] [diff] [review] not_null_pressure_ver2.diff smaug is your best reviewer for this -- he's already r?'d, so I'd just wait for that.
Attachment #8396202 -
Flags: review?(vladimir)
Comment 5•10 years ago
|
||
Comment on attachment 8396202 [details] [diff] [review] not_null_pressure_ver2.diff > event.button = button; >+ event.pressure = event.buttons ? 0.5f : 0.0f; > event.convertToPointer = mouseEvent->convertToPointer = false; Key point is hardware that does not support pressure, how do we check here if hardware does or does not support pressure? I know for example N900 with Gtk backend, when Stylus or Finger is used generate Mouse events with different Pressure value (non 0.0) and here we going just kill that value and ignore it... does not look good to me
Attachment #8396202 -
Flags: feedback?(oleg.romashin) → feedback-
Comment 6•10 years ago
|
||
Comment on attachment 8396202 [details] [diff] [review] not_null_pressure_ver2.diff Indeed. I would still but the stuff to WidgetPointerEvent implementation. It could have a getter for pressure which returns 0 if there are no buttons, and setting buttons could set pressure to 0.5, unless pressure is already set. Something like that.
Attachment #8396202 -
Flags: review?(bugs) → review-
Assignee | ||
Comment 7•10 years ago
|
||
+ changes with saving pressure in case it is exist
Attachment #8396202 -
Attachment is obsolete: true
Attachment #8396202 -
Flags: feedback?(nicklebedev37)
Attachment #8406005 -
Flags: review?(pavlov)
Attachment #8406005 -
Flags: review?(bugs)
Attachment #8406005 -
Flags: feedback?(oleg.romashin)
Attachment #8406005 -
Flags: feedback?(nicklebedev37)
Assignee | ||
Comment 8•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=a0e04253ca66
Comment 9•10 years ago
|
||
Comment on attachment 8406005 [details] [diff] [review] not_null_pressure_ver3.diff (Stuart isn't an active contributor these days and should be removed from the "suggested reviewers" list.)
Attachment #8406005 -
Flags: review?(pavlov)
Comment 10•10 years ago
|
||
Comment on attachment 8406005 [details] [diff] [review] not_null_pressure_ver3.diff > event.button = button; >+ event.pressure = event.buttons >+ ? mouseEvent->pressure ? mouseEvent->pressure : 0.5f >+ : 0.0f; It is a bit hard to read... maybe re-indent
Attachment #8406005 -
Flags: feedback?(oleg.romashin) → feedback+
Assignee | ||
Comment 11•10 years ago
|
||
> It is a bit hard to read... maybe re-indent Is this variant better? > >+ event.pressure = event.buttons ? > >+ mouseEvent->pressure ? mouseEvent->pressure : 0.5f : > >+ 0.0f;
Comment 12•10 years ago
|
||
yes, that would look better
Updated•10 years ago
|
Attachment #8406005 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 13•10 years ago
|
||
+ changes according with comments
Attachment #8406005 -
Attachment is obsolete: true
Attachment #8406005 -
Flags: feedback?(nicklebedev37)
Attachment #8407378 -
Flags: review?(bugs)
Attachment #8407378 -
Flags: feedback?(oleg.romashin)
Comment 14•10 years ago
|
||
Comment on attachment 8407378 [details] [diff] [review] not_null_pressure_ver4.diff > WidgetPointerEvent event(*mouseEvent); > event.message = pointerMessage; > event.button = button; >+ event.pressure = event.buttons ? >+ mouseEvent->pressure ? mouseEvent->pressure : 0.5f : >+ 0.0f; Actually, add 2 spaces before mouseEvent and 0.0f Tests please
Attachment #8407378 -
Flags: review?(bugs) → review+
Comment 15•10 years ago
|
||
Actually, doesn't the patch still miss got/lostpointercapture. Per spec those should have pressure 0.5 too in case there are active buttons.
Updated•10 years ago
|
Attachment #8407378 -
Flags: feedback?(oleg.romashin) → feedback+
Updated•10 years ago
|
Keywords: checkin-needed
Comment 16•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a12ca6c8b89b
Assignee: nobody → alessarik
Keywords: checkin-needed
Comment 17•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a12ca6c8b89b
Status: UNCONFIRMED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Comment 18•10 years ago
|
||
Looks like my comment 14 was ignored here. Maxim, will we get tests for this once the w3c tests have been reviewed, or do we need something here? We must get tests before enabling pointer events.
Flags: needinfo?(alessarik)
Assignee | ||
Comment 19•10 years ago
|
||
All tests were attached by me to Bug 1000870. If I get new tests or changes, I will update attaches. Also I try make all tests near with reviewed versions. If You want get only reviewed tests, and You can accelerate review-process, it will be good for all.
Comment 20•10 years ago
|
||
(It just doesn't make sense to import non-reviewed tests, which will possibly change real soon during the review process. Then we'd need to update them in our tree.) Ah, I see tests for 0.5 handling in Bug 1000870, thanks.
Updated•10 years ago
|
Flags: needinfo?(alessarik)
You need to log in
before you can comment on or make changes to this bug.
Description
•