Bug 219930 - GamepadButton.prototype is missing 'touched' attribute
Summary: GamepadButton.prototype is missing 'touched' attribute
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: Safari 14
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-12-15 17:17 PST by Frank Olivier
Modified: 2021-01-25 16:02 PST (History)
12 users (show)

See Also:


Attachments
Patch (20.70 KB, patch)
2020-12-16 09:56 PST, frankolivier
no flags Details | Formatted Diff | Diff
Patch (39.58 KB, patch)
2020-12-29 12:19 PST, frankolivier
no flags Details | Formatted Diff | Diff
Patch (41.60 KB, patch)
2021-01-02 21:50 PST, frankolivier
no flags Details | Formatted Diff | Diff
Patch (41.68 KB, patch)
2021-01-02 22:47 PST, frankolivier
no flags Details | Formatted Diff | Diff
Patch (41.68 KB, patch)
2021-01-03 15:24 PST, frankolivier
no flags Details | Formatted Diff | Diff
Patch (41.35 KB, patch)
2021-01-06 15:51 PST, frankolivier
no flags Details | Formatted Diff | Diff
Patch (57.02 KB, patch)
2021-01-25 13:54 PST, frankolivier
no flags Details | Formatted Diff | Diff
Patch (58.88 KB, patch)
2021-01-25 15:24 PST, frankolivier
no flags Details | Formatted Diff | Diff
Patch (58.96 KB, patch)
2021-01-25 16:02 PST, frankolivier
frankolivier: review?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Frank Olivier 2020-12-15 17:17:06 PST
GamepadButton.prototype is missing 'touched' attribute in WebKit

Chrome, Firefox both have the 'touched' attribute

From https://w3c.github.io/gamepad/#dom-gamepadbutton-touched

touched attribute
The touched state of the button. If the button is capable of detecting touch, this property MUST be true if the button is currently being touched, and false otherwise. If the button is not capable of detecting touch and is capable of reporting an analog value, this property MUST be true if the value property is greater than 0, and false if the value is 0. If the button is not capable of detecting touch and can only report a digital value, this property MUST mirror the pressed attribute.
Comment 1 frankolivier 2020-12-16 09:56:57 PST
Created attachment 416344 [details]
Patch
Comment 2 Dean Jackson 2020-12-16 10:03:01 PST
Comment on attachment 416344 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=416344&action=review

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

Please remove this line. You don't need to list a new test since you've modified an existing one.

> Source/WebCore/Modules/gamepad/GamepadButton.cpp:50
> +    If the button is capable of detecting touch, this property MUST be true if the button is currently being touched, and false otherwise. If the button is not capable of detecting touch and is capable of reporting an analog value, this property MUST be true if the value property is greater than 0, and false if the value is 0. If the button is not capable of detecting touch and can only report a digital value, this property MUST mirror the pressed attribute.

Please wrap this line.

Also, I think we need to add a comment on the implementation  e.g.

FIXME: Update if/when we can support touches on a controller.
Comment 3 Brady Eidson 2020-12-16 10:04:59 PST
Comment on attachment 416344 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=416344&action=review

> Source/WebCore/Modules/gamepad/GamepadButton.cpp:52
> +    /*
> +    https://www.w3.org/TR/gamepad/#dom-gamepadbutton-touched
> +    If the button is capable of detecting touch, this property MUST be true if the button is currently being touched, and false otherwise. If the button is not capable of detecting touch and is capable of reporting an analog value, this property MUST be true if the value property is greater than 0, and false if the value is 0. If the button is not capable of detecting touch and can only report a digital value, this property MUST mirror the pressed attribute.
> +    */
> +    return pressed();

Is there something we can do better here in GCF-land? Does it support touch sensitivity yet? (That was the original motivation here, to allow distinguishing touched and pressed)
Comment 4 Brady Eidson 2020-12-16 10:14:10 PST
(In reply to Brady Eidson from comment #3)
> Comment on attachment 416344 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=416344&action=review
> 
> > Source/WebCore/Modules/gamepad/GamepadButton.cpp:52
> > +    /*
> > +    https://www.w3.org/TR/gamepad/#dom-gamepadbutton-touched
> > +    If the button is capable of detecting touch, this property MUST be true if the button is currently being touched, and false otherwise. If the button is not capable of detecting touch and is capable of reporting an analog value, this property MUST be true if the value property is greater than 0, and false if the value is 0. If the button is not capable of detecting touch and can only report a digital value, this property MUST mirror the pressed attribute.
> > +    */
> > +    return pressed();
> 
> Is there something we can do better here in GCF-land? Does it support touch
> sensitivity yet? (That was the original motivation here, to allow
> distinguishing touched and pressed)

(I know this is basically what your comment says, but what I'm suggesting is make sure GameController.framework doesn't already offer it somehow)
Comment 5 Radar WebKit Bug Importer 2020-12-22 12:39:20 PST
<rdar://problem/72596955>
Comment 6 frankolivier 2020-12-29 12:19:20 PST
Created attachment 416841 [details]
Patch
Comment 7 frankolivier 2021-01-02 21:50:56 PST
Created attachment 416906 [details]
Patch
Comment 8 frankolivier 2021-01-02 22:47:33 PST
Created attachment 416907 [details]
Patch
Comment 9 frankolivier 2021-01-03 15:24:20 PST
Created attachment 416917 [details]
Patch
Comment 10 Dean Jackson 2021-01-05 12:20:32 PST
Comment on attachment 416917 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=416917&action=review

> Source/WebKit/Shared/Gamepad/GamepadData.cpp:46
> +, m_buttonValues(WTF::map(buttonValues, [](const auto& button) { return button.value(); }))
> +, m_buttonTouchedValues(WTF::map(buttonValues, [](const auto& button) { return button.touched(); }))
> +, m_buttonPressedValues(WTF::map(buttonValues, [](const auto& button) { return button.pressed(); }))

Nit: formatting error here.

> Source/WebKit/Shared/Gamepad/GamepadData.h:72
>      Vector<double> m_buttonValues;
> +    Vector<bool> m_buttonTouchedValues;
> +    Vector<bool> m_buttonPressedValues;

I wonder if we should have a struct with value, touchedValue and PressedValue, and then just a Vector of those?

> Source/WebKit/WebProcess/Gamepad/WebGamepad.cpp:73
> +        m_buttonValues[i] = SharedGamepadButtonValue(gamepadData.buttonValues()[i], gamepadData.buttonPressedValues()[i], gamepadData.buttonTouchedValues()[i]);

should we ASSERT that buttonValues + buttonPressedValues + buttonTouchedValues all have the same size?
Comment 11 frankolivier 2021-01-06 15:51:04 PST
Created attachment 417136 [details]
Patch
Comment 12 Darin Adler 2021-01-06 16:00:56 PST
Comment on attachment 417136 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417136&action=review

> Source/WebCore/platform/gamepad/SharedGamepadValue.h:133
> +    /*
> +    template<class Decoder>
> +    static Optional<MonotonicTime> decode(Decoder& decoder)
> +    {
> +        Optional<double> time;
> +        decoder >> time;
> +        if (!time)
> +            return WTF::nullopt;
> +        return MonotonicTime::fromRawSeconds(*time);
> +    }
> +    */

Normally we don’t include commented-out code.
Comment 13 Darin Adler 2021-01-06 17:00:04 PST
Comment on attachment 417136 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417136&action=review

Generally looks good, but we can do something simpler.

> Source/WebCore/platform/gamepad/SharedGamepadValue.h:67
> +class SharedGamepadButtonValue : public SharedGamepadValue {

I wish this was more like a struct and same for SharedGamepadValue. All these getters and setters don’t seem helpful. These are simple data holders.

> Source/WebCore/platform/gamepad/SharedGamepadValue.h:70
> +        : SharedGamepadValue()

Should be able to omit this.

> Source/WebCore/platform/gamepad/SharedGamepadValue.h:71
> +        , m_simulate(true)

This leaves m_touched and m_pressed uninitialized. If someone then calls setTouched, then the uninitialized value of m_pressed is "exposed". Let’s just always initialize.

After studying this, I came to the conclusion we don’t need m_simulate at all.

> Source/WebCore/platform/gamepad/SharedGamepadValue.h:77
> +        , m_simulate(true)

Instead we should just initialize like this:

    , m_touched(value > 0.1)
    , m_pressed(m_touched)

That will work.

> Source/WebCore/platform/gamepad/SharedGamepadValue.h:81
> +    explicit SharedGamepadButtonValue(double value, bool touched, bool pressed)

No need for "explicit" here.

> Source/WebCore/platform/gamepad/SharedGamepadValue.h:120
> +        encoder << value();
> +        encoder << touched();
> +        encoder << pressed();

A little strange that encoding strips away the "simulated" state.

> Source/WebCore/platform/gamepad/SharedGamepadValue.h:154
> +    bool m_touched;
> +    bool m_pressed;
> +    bool m_simulate;

These should be initialized here. I suggest false/false/false.

    bool m_touched { false };

etc.

Also, the name should probably be m_simulated, not m_simulate.

> Source/WebKit/Shared/Gamepad/GamepadData.h:61
> -    const Vector<double>& buttonValues() const { return m_buttonValues; }
> +    const Vector<WebCore::SharedGamepadButtonValue>& buttonValues() const { return m_buttonValues; }

Seems like the sharing here is very strange. Why do these values need to be shared, and not just a struct?
Comment 14 frankolivier 2021-01-25 13:54:34 PST
Created attachment 418331 [details]
Patch
Comment 15 frankolivier 2021-01-25 15:24:28 PST
Created attachment 418345 [details]
Patch
Comment 16 frankolivier 2021-01-25 16:02:17 PST
Created attachment 418354 [details]
Patch