1311450 - Only show the 'touch' padding for the select dropdown when opened via touch

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

From https://bugzilla.mozilla.org/show_bug.cgi?id=1091592#c35,
> On a touch-enabled device, the dropdowns should only have the additional touch-
> friendly padding if they've been enabled through a touch event. When activated 
> using the mouse, they should use the normal element size with less padding.

We can fix this bug changing nsListControlFrame::MouseDown to check mozInputSource to see if it is MOZ_SOURCE_TOUCH and then passing in an extra flag to FireShowDropdown if the source was a touch. This can then apply different styling by setting an attribute on the #ContentSelectDropdown to mark that it was opened from a touch event.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1311450 - Only show the 'touch' padding for the select dropdown when opened via touch.

Review commit: https://reviewboard.mozilla.org/r/87166/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/87166/

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8802896 [details]
Bug 1311450 - Only show the 'touch' padding for the select dropdown when opened via touch.

https://reviewboard.mozilla.org/r/87166/#review86234

This is ok to me, if this is ok to mconley.



(Hmm, random question. Would it make sense to add some helper event class for chrome stuff. Perhaps just reuse DataContainerEvent, but make its use easy from C++ and from JS. Then this kind of change wouldn't change the event type, but just add some random flag on the event and listener could call something like event.getData("sourceIsTouch"))

::: layout/forms/nsListControlFrame.cpp:1780
(Diff revision 1)
>  
>    return NS_ERROR_FAILURE;
>  }
>  
>  static bool
> -FireShowDropDownEvent(nsIContent* aContent, bool show)
> +FireShowDropDownEvent(nsIContent* aContent, bool show, bool isSourceTouchEvent)

nit, aIsSourceTouchEvent

and while you're here, want to fix also show to be
aShow

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8802896 [details]
Bug 1311450 - Only show the 'touch' padding for the select dropdown when opened via touch.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87166/diff/1-2/

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8802896 [details]
Bug 1311450 - Only show the 'touch' padding for the select dropdown when opened via touch.

https://reviewboard.mozilla.org/r/87166/#review86368

Thanks jaws!

::: layout/forms/nsListControlFrame.cpp:1784
(Diff revision 2)
> +    nsString eventName;
> +    if (aShow) {
> +      eventName = aIsSourceTouchEvent ? NS_LITERAL_STRING("mozshowdropdown-sourcetouch") :
> +                                        NS_LITERAL_STRING("mozshowdropdown");
> +    } else {
> +      eventName = NS_LITERAL_STRING("mozhidedropdown");
> +    }

It'd kinda be nice if we could set some kind of "sourcetouch" detail to true on the event instead of creating a whole new event just for this. I guess that's not really easy from the native code though. :/

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Thanks for the quick reviews!

(In reply to Mike Conley (:mconley) - (high latency on reviews and needinfos) from comment #4)
> It'd kinda be nice if we could set some kind of "sourcetouch" detail to true
> on the event instead of creating a whole new event just for this. I guess
> that's not really easy from the native code though. :/

Yeah, this is what Olli was referring to in comment 2. I'll file a follow-up bug to implement that.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a97c0531025f
Only show the 'touch' padding for the select dropdown when opened via touch. r=mconley,smaug

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a97c0531025f