Closed Bug 1338961 Opened 8 years ago Closed 8 years ago

RFE: A feature allow anonymous HTML content to ignore defaultPrevented flag

Categories

(Core :: DOM: UI Events & Focus Handling, defect, P1)

Product:
Core
Component:
DOM: UI Events & Focus Handling
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox53 + fixed
firefox54 --- verified

People

(Reporter: ralin, Assigned: timdream)

References

Details

Attachments

(2 files)

mozignorepreventdefault.patch
8.67 KB, patch
Details | Diff | Splinter Review
Bug 1338961 - A mozinputrangeignorepreventdefault hack for input[type=range], , sr=smaug
59 bytes, text/x-review-board-request
smaug
: review+
gchang
: approval-mozilla-aurora+
Details
TLDR: There is `mozSystemGroup` to handle events from XBL separately (and disregard stopPropagation), but there are no mechanism that prevents web-content’s `preventDefault` from breaking anonymous content’s default behavior. We would need that to fix bug 1327097 and possible future use cases. ------ In Firefox 53, desktop video control visual refresh was landed with pure HTML implementation (bug 1271765). This might be one of the large scale De-XUL works we’ve done so far. We’ve fixed several inconsistent behaviors that was caused by the difference between HTML and XUL elements, such as a11y traversal, DOM event and layout reflow. However, these are not enough for new video control to behaves completely same as the old implementation. Bug 1327097 addresses a problem that web author can use `preventDefault` to disable default behavior within video control, i.e. the video timeline, <input type=“range”> refrain from seeking due to web author adding `onmousedown="return false"` to <body>. We’ve already taken a lot of advantages from XBL as an independent, self-contained and re-usable component. I’m not sure is it because we didn’t initially consider putting HTML into content or other historical reasons, the event system seems not totally separated from web-content for XBL. Video control should be deemed as a standalone and encapsulated native widget, which should not interfered with by any unintentional/intentional action from web author. Right now, we have `mozSystemGroup` to handle the events from XBL separately, but still lack a mechanism that prevents web-content’s `preventDefault` from breaking anonymous content’s default behavior. Should we consider adding a feature in event system to fix this kind of issue? Thanks
I don't understand this. If the event listener added to system event group doesn't want to care about defaultPrevented, it just doesn't check that. And browser implemented widgets aren't separated from the rest of the DOM, never have, and I see no reason why they would.
(In reply to Olli Pettay [:smaug] from comment #1) Please re-read the comments in bug 1327097. The problem is not about what XUL JS do when the callback is invoked in the system event group event dispatcher. The problem is about the default behavior of the HTML range input when mouse clicks. We asks the range input to continue to respond to mouse click (and update the range specified) even when the event is being preventDefault'd from content. (In reply to Olli Pettay [:smaug] from comment #2) > If needed, I think we could expose the attributes > http://searchfox.org/mozilla-central/rev/ > d3307f19d5dac31d7d36fc206b00b686de82eee4/dom/webidl/Event.webidl#42,44 > also in XBL. That won't help because it the mouse click should be continue to be handled in the native HTMLInputElement implementation.
Which features in native anonymous content shouldn't be affected by preventDefault() in web content and which should?
Now I'm lost. in comment 3 you say range should continue to respond to mouse click, but in comment 5 it shouldn't. But ok, there are some cases when web content should be able to prevent default handling and some cases when not. Do we have some list of especially the latter, when the web page should not be able to prevent the default handling?
(In reply to Olli Pettay [:smaug] from comment #6) > Now I'm lost. in comment 3 you say range should continue to respond to mouse > click, but in comment 5 it shouldn't. comment 5 was a typo. The seeker in video control of <video> should NOT be affected by preventDefault() in web content. > But ok, there are some cases when web content should be able to prevent > default handling and some cases when not. Do we have some list of especially > the latter, when the web page should not be able to prevent the default > handling? I don't have a list, and it seems that HTML-based video control is the first widget affected by this, so we could consider that as the first item on the list.
(In reply to Olli Pettay [:smaug] from comment #6) > Now I'm lost. in comment 3 you say range should continue to respond to mouse > click, but in comment 5 it shouldn't. > But ok, there are some cases when web content should be able to prevent > default handling and some cases when not. Do we have some list of especially > the latter, when the web page should not be able to prevent the default > handling? Thank you Tim and Olli, I don't have certain list or feature either. Could we assume that all the anonymous content(HTML element)'s default behavior should not be affected by preventDefault()?
ok, so which all events in video control should not care of the prevent default status set by web content? mousedown/up/click? keydown/press/up? wheel? touch*?
So far all the native anonymous element have just the normal event handling. Changing that would most probably break many things, like data:text/html,<select onmousedown="event.preventDefault()"><option>1<option>2</select>
(In reply to Olli Pettay [:smaug] from comment #10) > So far all the native anonymous element have just the normal event handling. > Changing that would most probably break many things, like > data:text/html,<select > onmousedown="event.preventDefault()"><option>1<option>2</select> yes, I understood. But it's more obvious that video controls is a composited element, so I'd see this a tiny bit confusing if preventDefault() breaks only seeker instead of whole UI. That is, if web user prevent click's default behavior, it make more sense to either disable all controls including play button, fullscreen button etc, or conversely retain all the functionality.
yes, I think preventDefault() should prevent all possible default handling. This is something to discuss with other browser vendors too.
But if there is some particular case where default handling should always happen, we could perhaps special case that? Like in HTML range implementation perhaps
I pieced together a patch to do what's suggested here. We can discuss about the naming. I had a quick chat with Hsinyi about this. I understand moz- extensions in our DOM impl can be a bad technical debt and a bad precedent, yet ultimately we would need a decision between shipping regression or managing technical debt. A transparent schedule around "proper" web platform solutions like Shadow DOM can also help the decision here. Thanks.
Flags: needinfo?(bugs)
Comment on attachment 8837024 [details] [diff] [review] mozignorepreventdefault.patch >+ bool MozIgnorePreventDefault() const >+ { >+ return GetBoolAttr(nsGkAtoms::mozignorepreventdefault); >+ } This is not ok when use of MozIgnorePreventDefault isn't limited to native anonymous content. We must not expose any of this behavior to the web pages. > attribute DOMString useMap; > readonly attribute nsIControllers controllers; >+ attribute boolean mozIgnorePreventDefault; It is unclear which default handling should be prevented. All? Does that include all the listeners in system group, since those tend to do default handling.
Flags: needinfo?(bugs)
How does shadow DOM help here at all? By default shadow DOM does all the default handling. (and shadow DOM in native anonymous content is of course totally proprietary.)
Comment on attachment 8837024 [details] [diff] [review] mozignorepreventdefault.patch If we need this hacky solution, MozIgnorePreventDefault() should only return true in case document is a chrome document or the element is in native anonymous content.
Thanks for the reply. I don't understand DOM code enough to realize the HTML attribute is exposed in the content regardless of WebIDL flags! Will see how to limit it's behavior, given that ... As I already mentioned in comment 14, we should discuss the naming. I hope we can keep the hack small and only solve the use of input[type=range] in video control. How about |mozinputrangeignorepreventdefault|? I will ask you for review once I figure out how I can do comment 17 and adding a test for this.
Flags: needinfo?(bugs)
You don't expose the webidl part, but you expose the feature if one uses setAttribute. And still, what all default handling should happen, regardless of the preventDefault() ?
Flags: needinfo?(bugs)
Hi Olli, As I know, other controls in video aren't affected by this issue except slider(timeline and volume control). So I would say at least 'mousedown', 'mouseup' and 'mousemove' should happen in this case.
Priority: -- → P1
So given the recent comments in bug 1327097, should we just block certain events to propagate from media elements when "native" controls are being used? What do other browsers do? Is it only blink not propagating the events?
Comment on attachment 8838443 [details] Bug 1338961 - A mozinputrangeignorepreventdefault hack for input[type=range], , sr=smaug https://reviewboard.mozilla.org/r/113380/#review114904 All those fixed, r+ this temporary way to fix the regression, assuming nothing better comes from bug 1327097 or the spec bug.. Happy to review again, if you think there are enough changes that should be re-reviewed. ::: dom/html/HTMLInputElement.h:787 (Diff revision 2) > void MozSetFileArray(const Sequence<OwningNonNull<File>>& aFiles); > void MozSetDirectory(const nsAString& aDirectoryPath, ErrorResult& aRv); > > + bool MozInputRangeIgnorePreventDefault() const > + { > + return (IsInChromeDocument() || IsInAnonymousSubtree()) && Should be IsInNativeAnonymousSubtree, not IsInAnonymousSubtree ::: dom/html/HTMLInputElement.h:791 (Diff revision 2) > + { > + return (IsInChromeDocument() || IsInAnonymousSubtree()) && > + GetBoolAttr(nsGkAtoms::mozinputrangeignorepreventdefault); > + } > + > + void SetMozInputRangeIgnorePreventDefault(bool aValue, ErrorResult& aRv) There are no callers for this, so no need for this method. ::: dom/html/HTMLInputElement.cpp:1631 (Diff revision 2) > NS_IMPL_UINT_ATTR_NON_ZERO_DEFAULT_VALUE(HTMLInputElement, Size, size, DEFAULT_COLS) > NS_IMPL_STRING_ATTR(HTMLInputElement, Pattern, pattern) > NS_IMPL_STRING_ATTR(HTMLInputElement, Placeholder, placeholder) > NS_IMPL_ENUM_ATTR_DEFAULT_VALUE(HTMLInputElement, Type, type, > kInputDefaultType->tag) > +NS_IMPL_BOOL_ATTR(HTMLInputElement, MozInputRangeIgnorePreventDefault, mozinputrangeignorepreventdefault) Drop this ::: dom/html/HTMLInputElement.cpp:4929 (Diff revision 2) > HTMLInputElement::PostHandleEventForRangeThumb(EventChainPostVisitor& aVisitor) > { > MOZ_ASSERT(mType == NS_FORM_INPUT_RANGE); > > - if (nsEventStatus_eConsumeNoDefault == aVisitor.mEventStatus || > + if ((nsEventStatus_eConsumeNoDefault == aVisitor.mEventStatus && > + !MozInputRangeIgnorePreventDefault()) || odd indentation. I guess one extra space before ! ::: dom/interfaces/html/nsIDOMHTMLInputElement.idl:95 (Diff revision 2) > attribute DOMString selectionDirection; > > > attribute DOMString useMap; > readonly attribute nsIControllers controllers; > + attribute boolean mozInputRangeIgnorePreventDefault; .idl is deprecated for DOM stuff. No need to add anything here. ::: dom/webidl/HTMLInputElement.webidl:191 (Diff revision 2) > // This function will return null if @autocomplete is not defined for the > // current @type > AutocompleteInfo? getAutocompleteInfo(); > + > + [Func="IsChromeOrXBL", Pure, SetterThrows] > + attribute boolean mozInputRangeIgnorePreventDefault; We don't actually need to add anything to webidl, since no one from .js is accessing this. Just keep the C++ side.
Attachment #8838443 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] (pto-ish for couple of days) from comment #23) > So given the recent comments in bug 1327097, should we just block certain > events to propagate from media elements when "native" controls are being > used? > What do other browsers do? Is it only blink not propagating the events? Let's take these conversation back to bug 1327097. I am not entirely sure if a new approach outlined there can make it to fx53, so I prefer we land and uplift the patch here and land a proper fix there.
Assignee: nobody → timdream
Status: NEW → ASSIGNED
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/aab681ba77bf A mozinputrangeignorepreventdefault hack for input[type=range], r=smaug, sr=smaug
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/aab681ba77bf
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
[Tracking Requested - why for this release]: This needs to land in fx53 to workaround bug 1327097.
status-firefox53: --- → affected
tracking-firefox53: --- → ?
Can you request uplift on the patch, to aurora? Thanks!
tracking-firefox53: ? → +
Flags: needinfo?(timdream)
Comment on attachment 8838443 [details] Bug 1338961 - A mozinputrangeignorepreventdefault hack for input[type=range], , sr=smaug Approval Request Comment [Feature/Bug causing the regression]: Bug 1271765, update video element control UI [User impact if declined]: As described in bug 1327097 comment 0, user will be blocked from using the seeking control if the page preventDefault'd the mouse event. [Is this code covered by automated tests?]: Yes, with tests proving the attribute work and with attribute only added onto the video control. [Has the fix been verified in Nightly?]: Not yet. [Needs manual test from QE? If yes, steps to reproduce]: No. If needed, look at the original bug for STR. [List of other uplifts needed for the feature/fix]: Just this one. [Is the change risky?]: No. [Why is the change risky/not risky?]: Shouldn't be as it's a localized fix. [String changes made/needed]: No.
Flags: needinfo?(timdream)
Attachment #8838443 - Flags: approval-mozilla-aurora?
Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(brindusa.tot)
Build ID: 20170223030204 User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0 Verified as fixed on Firefox Nightly 54.0a1 on Windows 10 x 64, Mac OS X 10.11 and Ubuntu 16.04 x64 on.
Status: RESOLVED → VERIFIED
status-firefox54: fixed → verified
Flags: needinfo?(brindusa.tot)
Comment on attachment 8838443 [details] Bug 1338961 - A mozinputrangeignorepreventdefault hack for input[type=range], , sr=smaug Fix the seeking control issue in UI when page preventDefault'd the mouse event. Aurora53+ and was verified.
Attachment #8838443 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: