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)

x86_64
Windows 8
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: alessarik, Assigned: alessarik)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Attached patch not_null_pressure_ver1.diff (obsolete) — Splinter Review
+ 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 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-
Component: Untriaged → Widget
Product: Firefox → Core
Attached patch not_null_pressure_ver2.diff (obsolete) — Splinter Review
(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 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 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-
Attached patch not_null_pressure_ver3.diff (obsolete) — Splinter Review
+ 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)
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 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+
> 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;
yes, that would look better
Attachment #8406005 - Flags: review?(bugs) → review+
+ 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 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+
Actually, doesn't the patch still miss got/lostpointercapture. Per spec those should
have pressure 0.5 too in case there are active buttons.
Attachment #8407378 - Flags: feedback?(oleg.romashin) → feedback+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a12ca6c8b89b
Status: UNCONFIRMED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Blocks: 822898
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)
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.
(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.
Flags: needinfo?(alessarik)
Blocks: 1292062
You need to log in before you can comment on or make changes to this bug.