Bug 188913 – Add getModifierState to MouseEvent

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Ryosuke Niwa]

Right now, getModifierState only exists on KeyboardEvent but it's also supposed to be on MouseEvent.

[Radar WebKit Bug Importer]

<rdar://problem/43668772>

[Ryosuke Niwa]

Created attachment 347985 [details]
Adds the method

[Ryosuke Niwa]

Created attachment 347989 [details]
Fixed iOS tests

[Simon Fraser (smfr)]

Comment on attachment 347989 [details]
Fixed iOS tests

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

> Source/WebCore/ChangeLog:13
> +        This patch also fixes the bug r235158 purposefully preserved that initMouseEvent was not clearing
> +        AltGraph and CapsLock states.

This is not a readable sentence.

> Source/WebCore/dom/UIEventWithKeyState.cpp:57
> +    if (keyIdentifier == "Control")
> +        return ctrlKey();
> +    if (keyIdentifier == "Shift")
> +        return shiftKey();
> +    if (keyIdentifier == "Alt")
> +        return altKey();
> +    if (keyIdentifier == "Meta")
> +        return metaKey();
> +    if (keyIdentifier == "AltGraph")
> +        return altGraphKey();
> +    if (keyIdentifier == "CapsLock")
> +        return capsLockKey();

Are these supposed to be case-sensitive comparisons? Can we use AtomicStrings here? Should the standard names be constants in the code?

> LayoutTests/ChangeLog:10
> +        Added two tests for getModifierState: one manually setting modifier key states in MouseEvent's constructor,
> +        and another one for dblclick inheriting modifier key states from the click event.

Are there really no WPT tests for these?

[Ryosuke Niwa]

(In reply to Simon Fraser (smfr) from comment #4)
> Comment on attachment 347989 [details]
> Fixed iOS tests
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=347989&action=review
> 
> > Source/WebCore/ChangeLog:13
> > +        This patch also fixes the bug r235158 purposefully preserved that initMouseEvent was not clearing
> > +        AltGraph and CapsLock states.
> 
> This is not a readable sentence.

Fixed per in-person discussion.

> > Source/WebCore/dom/UIEventWithKeyState.cpp:57
> > +    if (keyIdentifier == "Control")
> > +        return ctrlKey();
> > +    if (keyIdentifier == "Shift")
> > +        return shiftKey();
> > +    if (keyIdentifier == "Alt")
> > +        return altKey();
> > +    if (keyIdentifier == "Meta")
> > +        return metaKey();
> > +    if (keyIdentifier == "AltGraph")
> > +        return altGraphKey();
> > +    if (keyIdentifier == "CapsLock")
> > +        return capsLockKey();
> 
> Are these supposed to be case-sensitive comparisons? Can we use
> AtomicStrings here? Should the standard names be constants in the code?

The spec doesn't specify but both Chrome & Firefox does case-sensitive comparison as we do. I don't think it makes senes to use AtomicString her since the input to getModifierState isn't necessarily AtomicString.

> > LayoutTests/ChangeLog:10
> > +        Added two tests for getModifierState: one manually setting modifier key states in MouseEvent's constructor,
> > +        and another one for dblclick inheriting modifier key states from the click event.
> 
> Are there really no WPT tests for these?

Unfortunately not. There are literally no getModifierState tests other than IDL and one test Microsoft added.

[Ryosuke Niwa]

Filed https://github.com/w3c/uievents/issues/211 to the case sensitive comparison in the spec.

[Ryosuke Niwa]

Committed r235329: <https://trac.webkit.org/changeset/235329>

[Lucas Forschler]

Mass move bugs into the DOM component.