Closed Bug 752548 Opened 8 years ago Closed 7 years ago

Use capturing phase for notification.xml handlers

Categories

(SeaMonkey :: UI Design, defect, P2, major)

Tracking

(seamonkey2.9 wontfix, seamonkey2.10 fixed, seamonkey2.11 fixed, seamonkey2.12 fixed)

RESOLVED FIXED
seamonkey2.12
Tracking Status
seamonkey2.9 --- wontfix
seamonkey2.10 --- fixed
seamonkey2.11 --- fixed
seamonkey2.12 --- fixed

People

(Reporter: sgautherie, Assigned: sgautherie)

References

(Depends on 1 open bug)

Details

(Whiteboard: [fixed in SM 2.10: Av1a; SM 2.11: Bv1; SM 2.17: Cv1])

Attachments

(3 files)

Actually all the notification event handlers should probably use capturing.
I checked all handlers in this file:
*Plugin*:
 as filed.
*DOMUpdatePageReport,
 npapi-carbon-event-model-failure,
 pageshow:
 SeaMonkey always had them reversed compared to what Firefox always had.
 I don't know why that should be, but please double-check.
Attachment #621664 - Flags: review?(neil)
(In reply to neil@parkwaycc.co.uk from comment #1)
> Actually all the notification event handlers should probably use capturing.

That's how I would understand bug 335586, but I was surprised it is not the case :-|


(In reply to Serge Gautherie (:sgautherie) from comment #2)
> *DOMUpdatePageReport,

Maybe we can keep capturing this one on SM ftb,
then I'll file FF bugs for this event and MozApplicationManifest?
Comment on attachment 621664 [details] [diff] [review]
(Av1) Fix phase on notification.xml handlers
[Checked in: See comments 5, 10, 15]

>-      <handler event="DOMUpdatePageReport" phase="capturing">
>+      <handler event="DOMUpdatePageReport">
I don't think we should remove "capturing" unless there's a specific reason.
r=me with that fixed.
I'd also prefer to make MozApplicationManifest capturing, but you can do that as a followup if you want to fix Firefox first.

Note: npapi-carbon-event-model-failure is also actually a plugin event.
Attachment #621664 - Flags: review?(neil) → review+
Comment on attachment 621664 [details] [diff] [review]
(Av1) Fix phase on notification.xml handlers
[Checked in: See comments 5, 10, 15]

http://hg.mozilla.org/comm-central/rev/a0df19ed338c
Av1, with comment 4 suggestion(s).


[Approval Request Comment]
No risk, resync' with FF, and fix bug 647875.
Attachment #621664 - Attachment description: (Av1) Fix phase on notification.xml handlers → (Av1) Fix phase on notification.xml handlers [Checked in: See comment 4]
Attachment #621664 - Flags: approval-comm-beta?
Attachment #621664 - Flags: approval-comm-aurora?
(In reply to neil@parkwaycc.co.uk from comment #4)
> Note: npapi-carbon-event-model-failure is also actually a plugin event.

Should we add a '#ifdef XP_MACOSX' as Firefox has?
Or at least a comment, fwiw?
Severity: normal → major
Summary: Use capturing phase for plugin events → Use capturing phase for notification.xml handlers
Depends on: 752761
>> Note: npapi-carbon-event-model-failure is also actually a plugin event.
> Should we add a '#ifdef XP_MACOSX' as Firefox has?
> Or at least a comment, fwiw?
Well on non-OSX platforms this event will never be generated so it's a NOOP.
(In reply to Philip Chee from comment #7)
> Well on non-OSX platforms this event will never be generated so it's a NOOP.

Exactly: NOOP as in bloat/confusing.
Comment on attachment 621664 [details] [diff] [review]
(Av1) Fix phase on notification.xml handlers
[Checked in: See comments 5, 10, 15]

On beta at least we don't have a ClickToPlay or pageshow event handler, so I am omitting them from my upcoming checkin
Attachment #621664 - Flags: approval-comm-beta?
Attachment #621664 - Flags: approval-comm-beta+
Attachment #621664 - Flags: approval-comm-aurora?
Attachment #621664 - Flags: approval-comm-aurora+
Comment on attachment 621664 [details] [diff] [review]
(Av1) Fix phase on notification.xml handlers
[Checked in: See comments 5, 10, 15]

(In reply to Justin Wood (:Callek) from comment #9)
> On beta at least we don't have a ClickToPlay or pageshow event handler, so I
> am omitting them from my upcoming checkin

Right: bug 743312, which added both, didn't land on 2.10.

***

http://hg.mozilla.org/releases/comm-beta/rev/cd00c74eef12

It looks like you pushed the patch rather than the changeset :-<
Please, revert the DOMUpdatePageReport hunk.
Attachment #621664 - Attachment description: (Av1) Fix phase on notification.xml handlers [Checked in: See comment 4] → (Av1) Fix phase on notification.xml handlers [Checked in: See comment 4 & 10]
Keywords: checkin-needed
Whiteboard: [c-n: a0df19ed338c to c-a, comment 10 to c-b]
[Approval Request Comment]
Low risk, be consistent with other handlers.
Attachment #625815 - Flags: review?(neil)
Attachment #625815 - Flags: approval-comm-beta?
Attachment #625815 - Flags: approval-comm-aurora?
Attachment #625815 - Flags: review?(neil) → review+
Comment on attachment 625815 [details] [diff] [review]
(Bv1) Fix phase on (notification.xml) MozApplicationManifest handler
[Checked in: Comments 12 and 16]

http://hg.mozilla.org/comm-central/rev/05ee1becb262
Attachment #625815 - Attachment description: (Bv1) Fix phase on (notification.xml) MozApplicationManifest handler → (Bv1) Fix phase on (notification.xml) MozApplicationManifest handler [Checked in: Comment 12]
These are some DOM* events outside notification.xml:
they succeeded on Firefox Try, but I'm not sure whether this is wanted too...
Attachment #626056 - Flags: review?(neil)
Comment on attachment 626056 [details] [diff] [review]
(Cv1) Fix/Sync' phase on DOMMouseScroll handlers [Checked in: Comment 21]

The DOMMouseScroll one is reasonable but the other two are just chrome events.
Attachment #625815 - Flags: approval-comm-beta?
Attachment #625815 - Flags: approval-comm-beta+
Attachment #625815 - Flags: approval-comm-aurora?
Attachment #625815 - Flags: approval-comm-aurora+
Whiteboard: [c-n: a0df19ed338c to c-a, comment 10 to c-b] → [c-n: a0df19ed338c, 05ee1becb262 to c-a, comment 10, 05ee1becb262 to c-b]
Whiteboard: [c-n: a0df19ed338c, 05ee1becb262 to c-a, comment 10, 05ee1becb262 to c-b] → [c-n: a0df19ed338c, 05ee1becb262 to c-a; comment 10, 05ee1becb262 to c-b]
Comment on attachment 621664 [details] [diff] [review]
(Av1) Fix phase on notification.xml handlers
[Checked in: See comments 5, 10, 15]

http://hg.mozilla.org/releases/comm-aurora/rev/1e676abebcc0
Attachment #621664 - Attachment description: (Av1) Fix phase on notification.xml handlers [Checked in: See comment 4 & 10] → (Av1) Fix phase on notification.xml handlers [Checked in: See comments 4, 10, 15]
Comment on attachment 625815 [details] [diff] [review]
(Bv1) Fix phase on (notification.xml) MozApplicationManifest handler
[Checked in: Comments 12 and 16]

http://hg.mozilla.org/releases/comm-aurora/rev/eac2a85ceccd
Attachment #625815 - Attachment description: (Bv1) Fix phase on (notification.xml) MozApplicationManifest handler [Checked in: Comment 12] → (Bv1) Fix phase on (notification.xml) MozApplicationManifest handler [Checked in: Comments 12 and 16]
Note: I intentionally didn't push the two code (!) changes to c-b here because I'm not sure about the risk assessment (don't want to be blamed for regressing Beta once again). I'll leave it to Callek to check with Neil and possibly push before tagging b3.
Whiteboard: [c-n: a0df19ed338c, 05ee1becb262 to c-a; comment 10, 05ee1becb262 to c-b] → [c-n: comment 10, 05ee1becb262 to c-b]
Attachment #621664 - Attachment description: (Av1) Fix phase on notification.xml handlers [Checked in: See comments 4, 10, 15] → (Av1) Fix phase on notification.xml handlers [Checked in: See comments 5, 10, 15]
Whiteboard: [c-n: comment 10, 05ee1becb262 to c-b] → [c-n: comment 10, 05ee1becb262 to c-b] [fixed in SM 2.10: Av1a; SM 2.11: Bv1]
2.10 has been tagged, clearing requests to land on c-b since that contains the fixes now that we're past the migration. Sorry Serge.
Keywords: checkin-needed
Whiteboard: [c-n: comment 10, 05ee1becb262 to c-b] [fixed in SM 2.10: Av1a; SM 2.11: Bv1] → [fixed in SM 2.10: Av1a; SM 2.11: Bv1]
Comment on attachment 626056 [details] [diff] [review]
(Cv1) Fix/Sync' phase on DOMMouseScroll handlers [Checked in: Comment 21]

r=me on the DOMMouseScroll events ONLY, because there we're interested in content events (the DOMMenu* events are all chrome events).

Also it doesn't look as if you've filed a bug on Firefox's zoom manager.
Attachment #626056 - Flags: review?(neil) → review+
Target Milestone: seamonkey2.12 → seamonkey2.13
Target Milestone: seamonkey2.13 → ---
Target Milestone: --- → seamonkey2.12
> r=me on the DOMMouseScroll events ONLY, because there we're interested in content
> events (the DOMMenu* events are all chrome events).
This patch has bit rotted. DOMMouseScroll events have been replaced by wheel events.
Whiteboard: [fixed in SM 2.10: Av1a; SM 2.11: Bv1] → [fixed in SM 2.10: Av1a; SM 2.11: Bv1][Cv1 patch bit-rotted]
> r=me on the DOMMouseScroll events ONLY, because there we're interested in content
> events (the DOMMenu* events are all chrome events).
Pushed only the DOMMouseScroll to comm-central:
http://hg.mozilla.org/comm-central/rev/57ba43a4ed08
Resolving as FIXED.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in SM 2.10: Av1a; SM 2.11: Bv1][Cv1 patch bit-rotted] → [fixed in SM 2.10: Av1a; SM 2.11: Bv1; SM 2.17: Cv1]
Attachment #626056 - Attachment description: (Cv1) Fix/Sync' phase on DOMMenuItemActive, DOMMenuItemInactive and DOMMouseScroll handlers → (Cv1) Fix/Sync' phase on DOMMouseScroll handlers [Checked in: Comment 21]
> Pushed only the DOMMouseScroll part to comm-central:
> http://hg.mozilla.org/comm-central/rev/57ba43a4ed08
Actually I fixed the bit-rot (DOMMouseScroll -> wheel) before checking in.
You need to log in before you can comment on or make changes to this bug.