2.56 KB, text/html
13.67 KB, patch
|Details | Diff | Splinter Review|
12.16 KB, patch
|Details | Diff | Splinter Review|
Do not prevent event dispatching even if there is no prescontext or (form) element is disabled. Currently for example mutation events don't work if form elements are disabled.
How would you prefer to handle it? a) change the early return NS_OK; to return super::PreHandleEvent(aVisitor); b) change the sense of the conditions and indent all the code c) extract the control-specific code into a separate function (named?)
Would it be enough to move the (disabled || !aVisitor || uiStyle->mUserInput == NS_STYLE_USER_INPUT_NONE || uiStyle->mUserInput == NS_STYLE_USER_INPUT_DISABLED) checks to PostHandleEvent and do default handling only if that expression evaluates to false.
Sadly form controls have to do some prehandling too. For instance, checkboxes need to update their value before the event propagates, but then need to reset their value if the event is subsequently cancelled.
(In reply to comment #3) > Sadly form controls have to do some prehandling too. For instance, checkboxes > need to update their value before the event propagates, but then need to reset > their value if the event is subsequently cancelled. > Of course. :( Well, then a) but perhaps adding flag to mItemFlags to indicate that posthandling should not be done?
Is there a way to indicate that only chrome event handlers should be called?
(In reply to comment #5) > Is there a way to indicate that only chrome event handlers should be called? > What is a "chrome event handler"? Atm there is no way to detect that an event listener was added in chrome. We could perhaps (at least in some cases) use 'trusted' flag for that, that is set by default when registering listeners in chrome.
(In reply to comment #6) >(In reply to comment #5) >>Is there a way to indicate that only chrome event handlers should be called? >What is a "chrome event handler"? That's a good question. I was going to say it was an event handler on a event target in an object managed by a docshell with chrome type. However your idea of firing only trusted event handlers is equally valid. For people not following the discussion on IRC the issue is that content does not expect to see events fired for disabled form controls. However this then causes bugs because our browser chrome (e.g. tooltips) does want to see them.
Some events should be fired, I think. mousemove, mouseover, mouseout? but not click, keypress? Jonas, I sent a mail to webapi list, but didn't get any comments. Could you raise the issue on a conf call?
I'm not really sure what we're doing with HTML specific event issues, i've raised that issue with the WG.
If our only concern is chrome vs content, don't we have the NO_CONTENT_DISPATCH flag for that? Note that I agree that we should fire mousemove.... click is a weird one; what do other browsers do?
(In reply to comment #10) >click is a weird one; what do other browsers do? IE seems not to generate a click event when you click on a disabled input.
Created attachment 234919 [details] [diff] [review] fixes mutation events and explicit event dispatching
Comment on attachment 234919 [details] [diff] [review] fixes mutation events and explicit event dispatching I think I want this :) Makes DOM Events (when dispatched using dispatchEvent) to work always. Also mutation events work and I think it is quite clear when an event is "presentation event", at least I hope so.
Comment on attachment 234919 [details] [diff] [review] fixes mutation events and explicit event dispatching - In content/events/public/nsEventDispatcher.h: /** + * mPresentationDispatch is PR_TRUE whenever event is dispatched by native Add "an" in front of "event". r=jst
Comment on attachment 234919 [details] [diff] [review] fixes mutation events and explicit event dispatching Boris, what do you think about this?
I'm not going to have time for this review until after Sept 3. :(
Comment on attachment 234919 [details] [diff] [review] fixes mutation events and explicit event dispatching >Index: content/events/public/nsEventDispatcher.h >@@ -98,16 +100,25 @@ public: >+ * mPresentationDispatch is PR_TRUE whenever event is dispatched by native >+ * code which handles the UI and layout. So it doesn't have to have anything to do with the presentation, then? For example, should DOMContentLoaded events have mPresentationDispatch true or false? What about DOMLinkAdded? If they're dispatched via nsContentUtils::DispatchTrustedEvent they won't default to the same thing as if they're dispatched via Dispatch() or DispatchEvent().... It feels like we need better docs here at least. > static nsresult Dispatch(nsISupports* aTarget, Document what the new arg means? For that matter, what does aCallback mean? And aEventStatus? How is aDOMEvent related to aEvent? I feel like I asked some of these when I was reviewing this code.... Can we possibly make the new arg not optional? It seems like the sort of thing where people will totally forget it's there, otherwise. > static nsresult DispatchDOMEvent(nsISupports* aTarget, Same here, esp. because this function doesn't have any optional args to start with... And again, document the arg. I'd like to see a diff -w of content/html/content/src; that should be much more reviewable.
Created attachment 272948 [details] some tests (synthetic event dispatch and also manually testable) Opera doesn't prevent synthetic events when controls are disabled, Safari does seem to prevent (our current behavior which doesn't make sense). In Opera if click() is called on disabled control, click event is dispatched - not sure whether we want to do that. Opera has some bug with disabled controls. After pressing some keys, disabled controls start to get keyup/down/press events.
Created attachment 272949 [details] [diff] [review] Disable event handling only when doing "full event dispatch" via ESM/Presshell (Mutation event handling without prescontext is bug 388746.)
Created attachment 272950 [details] [diff] [review] -w I don't know how else we could easily recognize that the event is really coming from the user, not a synthetic event dispatched by some script. The first patch for this bug had pretty much the same idea, but the implementation was much uglier.
So, people still need the fix.
Thomas, Olli does take his responsibilities seriously, as you can see by the number of bugs he fixes if you cared to look. Unfortunately, he has more on his plate than anyone could possibly have time for, so he has to prioritize. Please refrain from personal abuse in this bug database in the future; that sort of behavior is not acceptable here.
Boris, I fully respect the amount of work he, and everyone else for that matter, does - but it's pretty disappointing when I have my precious time wasted due to bugs that have been ASSIGNED for 10+ years to the same person without progress. Your own guidelines state that you should not mess with bugs assigned to other people, so maybe such bugs should then be unassigned and left for others to pick up? Instead of having one person just holding on to it for a decade, while everyone else ignores it? That's my opinion, and it's not a personal assault - it's a comment on what appears to be a broken issue tracking strategy, for a browser that is causing me and other web developers ever increasing pain. End of discussion.
Feel free to take this bug. And sorry that I've been busy with other stuff. (I'll try to find time for this if no one else picks this up.)
Hello all, This bug is really driving me crazy. Imagine the following situation: - You have a selectbox field that's disabled (since readonly attribute doesn't work as per html spec), I have no other standard option. - You need to have this value updated depending on what the user will do with the rest of the form's elements Check. Your only alternative is to try to emulate the disabled look n feel like onfocus=this.blur() along with a css. The only problem is that this currently doesn't work (bug https://bugzilla.mozilla.org/show_bug.cgi?id=1334155). There might also be some other work-around, but image that this has to apply to any other element too. And again, you never can be sure that the work-around won't be broken with the next version. So, checkmate. Please correct me if I'm missing something. Btw, chrome doesn't have problem dispatching events to disabled items. On the other hand the onfocus=this.blur() doesn't work at all and seems like they won't fix it.