nsEventStatus should be removed. There is really no good reason for nsEventStatus_eConsumeDoDefault and if that can be removed we can always just use NS_EVENT_FLAG_NO_DEFAULT and eventually after nsEvent/nsDOMEvent merge nsDOMEvent::preventDefault() and nsDOMEvent::isDefaultPrevented() (or whatever the method name will be). Doesn't exactly block bug 211916...

(In reply to comment #0) >nsDOMEvent::isDefaultPrevented() (or whatever the method name will be). What's wrong with the current getPreventDefault()?

(In reply to comment #1) > (In reply to comment #0) > >nsDOMEvent::isDefaultPrevented() (or whatever the method name will be). > What's wrong with the current getPreventDefault()? > That is in nsDOMUIEvent, not in nsDOMEvent.

Hey Smaug. Are you actively working on this? I see you have 18 bugs on your plate, so just wondering. If you don't think you'll get to it within the next week I'd be happy to have a stab at it. I note that we essentially check for nsEventStatus_eConsumeDoDefault in some parts of the code without doing it explicitly. E.g. http://lxr.mozilla.org/seamonkey/source/content/html/content/src/nsGenericHTMLElement.cpp#1446

(In reply to comment #3) > If you don't think you'll get to it within the next > week I'd be happy to have a stab at it. > Great!, because I'll be quite busy for a week or so.

Okay. I think the first step should be to eliminate all occurances of nsEventStatus_eConsumeDoDefault, and all code that acts differently for nsEventStatus_eIgnore and nsEventStatus_eConsumeDoDefault. If we do that first, we can be confident that when we replace mEventStatus with preventDefault/isDefaultPrevented separate we won't break anything and have to back out the *whole* patch.

Some low hanging fruit.

I believe these are the locations in the source that are of intest to us here.

Comment on attachment 216221 [details] [diff] [review] remove nsEventStatus_eConsumeDoDefault from widget code (checked in) checked in

So what does that patch actually do?

Comment on attachment 217231 [details] [diff] [review] nsGenericHTMLElement patch We want to get rid of eConsume*Do*Default. This patch gets rid of two pieces of code that make a distinction between eIgnore and eConsume*Do*Default. >- if ((aVisitor.mEventStatus == nsEventStatus_eIgnore || >- (aVisitor.mEventStatus != nsEventStatus_eConsumeNoDefault && >- (aVisitor.mEvent->message == NS_MOUSE_ENTER_SYNTH || >- aVisitor.mEvent->message == NS_MOUSE_EXIT_SYNTH)))) { >+ if (aVisitor.mEventStatus != nsEventStatus_eConsumeNoDefault) { This change would let in: * NS_MOUSE_LEFT_BUTTON_DOWN * NS_MOUSE_LEFT_CLICK * NS_UI_ACTIVATE * NS_KEY_PRESS * NS_FOCUS_CONTENT * NS_MOUSE_EXIT_SYNTH events with an mEventStatus of *Do*Default whereas it wouldn't have before. The NS_FOCUS_CONTENT and NS_MOUSE_EXIT_SYNTH shouldn't matter because they don't bubble. As for the other four, attachment 216225 [details] shows where we set eConsume*Do*Default, and from that and other searches I don't see any codepaths that would lead to these events comming through with a *Do*Default status (other than from another link element). Hence I don't think this change should break anything. What it does mean is that click events on a nested link will result in a few more events doing the rounds than otherwise would have occured. But again that's the nested link case. I don't think it's common or we should break much, if anything. >- aVisitor.mEventStatus = status; > } >- >- if (aVisitor.mEventStatus != nsEventStatus_eConsumeNoDefault) >- aVisitor.mEventStatus = nsEventStatus_eConsumeDoDefault; I don't think we should be caring about the return status of the dispatch of the NS_UI_ACTIVATE. The default action of the NS_MOUSE_LEFT_CLICK is to send a NS_UI_ACTIVATE. That's all. Why set the return status of the NS_MOUSE_LEFT_CLICK event based on what happened to the NS_UI_ACTIVATE event? I also haven't been able to see a reason for setting the *Do*Default. Looking at the points in the code where NS_MOUSE_LEFT_CLICK is checked, I don't see anything important that distinguishes between eIgnore and DoDefault. So I think the latter half of this second change shouldn't cause any noticable change in behaviour.

> The NS_FOCUS_CONTENT and NS_MOUSE_EXIT_SYNTH shouldn't matter because they > don't bubble. NS_FOCUS_CONTENT currently bubbles, last I checked, but that's something we'd like to fix. Thanks for explaining what the patch does; I'll try to review ASAP (probably tomorrow?).

No problem. I should really have done that up front.

Comment on attachment 217231 [details] [diff] [review] nsGenericHTMLElement patch >Index: mozilla/content/html/content/src/nsGenericHTMLElement.cpp > return NS_OK; Why not rv instead? It's already NS_OK. >+ if (!NS_IS_TRUSTED_EVENT(aVisitor.mEvent)) >+ return NS_OK; rv here too. And please put braces around the body; content style is to brace even single-line bodies after some mishaps with the other way. > if (target && IsArea(target) && !IsArea(this)) { >- // We are over an area and our element is not one. Return without >- // running anchor code. Why remove this comment? >- if ((aVisitor.mEventStatus == nsEventStatus_eIgnore || >- (aVisitor.mEventStatus != nsEventStatus_eConsumeNoDefault && >- (aVisitor.mEvent->message == NS_MOUSE_ENTER_SYNTH || >- aVisitor.mEvent->message == NS_MOUSE_EXIT_SYNTH)))) { >+ if (aVisitor.mEventStatus != nsEventStatus_eConsumeNoDefault) Note that we do currently (incorrectly) bubble focus events (except from textfields and textareas). Does that affect your analysis? I'm assuming "no". > rv = shell->HandleDOMEventWithTarget(this, &actEvent, &status); >- aVisitor.mEventStatus = status; Hmm. This is fun depending on what we want to do with the nested link stuff... I guess it's ok for now. sr=bzbarsky, but I'd really like smaug to have a look at this.

Comment on attachment 217231 [details] [diff] [review] nsGenericHTMLElement patch > case NS_MOUSE_LEFT_CLICK: >- if (nsEventStatus_eConsumeNoDefault != aVisitor.mEventStatus) { >+ if (aVisitor.mEvent->eventStructType == NS_MOUSE_EVENT) { > nsInputEvent* inputEvent = > NS_STATIC_CAST(nsInputEvent*, aVisitor.mEvent); > if (inputEvent->isControl || inputEvent->isMeta || > inputEvent->isAlt ||inputEvent->isShift) { > break; > } > > // The default action is simply to dispatch DOMActivate > nsIPresShell *shell = aVisitor.mPresContext->GetPresShell(); > if (shell) { > // single-click > nsUIEvent actEvent(NS_IS_TRUSTED_EVENT(aVisitor.mEvent), > NS_UI_ACTIVATE, 1); > nsEventStatus status = nsEventStatus_eIgnore; > > rv = shell->HandleDOMEventWithTarget(this, &actEvent, &status); >- aVisitor.mEventStatus = status; So what if the UI_ACTIVATE event is cancelled? I think aVisitor.mEventStatus = status should be left there. With that and bz' comment r=me

> Why not rv instead? It's already NS_OK. My thinking was that it's more immediately apparent what's being returned. You don't need to scan up to see if rv was set somewhere. But perhaps using rv compiles to smaller bytecode or something? > after some mishaps with the other way. Sure, but what sort of mishaps? I'd be interested to know how you can get it wrong. > Why remove this comment? Because it just diplicates the comment directly above it. Do you want it put back? > Does that affect your analysis? I'm assuming "no". Correct. I'm not saying my analysis couldn't conceivably be flawed of course. :-) > >- aVisitor.mEventStatus = status; > > Hmm. This is fun depending on what we want to do with the nested link > stuff... I guess it's ok for now. Actually I think whether we set mEventStatus to the DOMActivate's status is a separate issue. Depending on whether we want to allow nested links or not determines whether we want to preventDefault() at this point. If we want to allow nested links, we *have* to preventDefault() regardless of what happened to the DOMActivate. If we don't, the click event will go up to the next link and dispatch another DOMActivate targeted at the parent link. My opinion is that the default action of the click event on a link is to dispatch a DOMActivate event. Just because the user decides to preventDefault() the DOMActivate doesn't mean they want to do the same with the click. The two are separate events. The click should not be cancelled based on the success or failure of its default action. We don't preventDefault() based on the failure of default actions in other scenarios. I think setting the status of the click to the status of the DOMActivate it dispatches amounts to treating both events as a single event, and that that is simply wrong. If you both disagree I'll put it back, but I'd like to make sure that's what you really want if you don't mind.

(In reply to comment #16) > Actually I think whether we set mEventStatus to the DOMActivate's status is a > separate issue. I agree, it is a separate issue. So if you open a new bug (or perhaps there is already one open related to that) it is ok to me to remove aVisitor.mEventStatus = status. And bz already said "I guess it's ok for now."

Okay. I filed bug 333566. I'll explain the issue properly in a comment on that bug after this is checked in (so I can refer to the new code). I'll check in once bz has clarified what he wants me to do about the other comments I replied to in comment 16.

> But perhaps using rv compiles to smaller bytecode or something? Yeah.... :( > I'd be interested to know how you can get it wrong. Any time people are editing the code. We've had issues with comments between the if and the body confusing people, with misindented code looking like it is (or isn't!) in the body when the opposite was true, with patches accidentally moving statements out of the body, etc. It's possible to catch all this stuff in review, but that's much easier to do with bracing. > Do you want it put back? Nah, it's all good. ;) I'll admit that basically I don't have a good idea of how DOMActivate fits into things in general, so I'm not happy reviewing a change that changes the interaction with it. I'd have bryner review any patches to that effect...

IMNSHO returning NS_OK is much better than returning rv upon success. First of all it is much more expressive since it's clear what is being retured. Second, if someone adds or removes code above the return this might cause rv to get a new value.

Hey, you won't get any argument from me. :-) If you can convince bz I'll change it.

Sicking's convinced me; I'd say move the decl of rv down as far as possible and change all returns of rv above that to NS_OK.

I can't see myself doing any more on this for a while, so assigning to events@dom.bugs for now.