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

RESOLVED FIXED in Firefox 52

Status

()

Core
Layout: Form Controls
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

unspecified
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

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 hidden (mozreview-request)

Comment 2

10 months ago
mozreview-review
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
Attachment #8802896 - Flags: review?(bugs) → review+
Comment hidden (mozreview-request)

Comment 4

10 months ago
mozreview-review
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. :/
Attachment #8802896 - Flags: review?(mconley) → review+
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

10 months ago
Pushed by jwein@mozilla.com:
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

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a97c0531025f
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.