Use capturing phase for notification.xml handlers

RESOLVED FIXED in seamonkey2.12

Status

SeaMonkey
UI Design
P2
major
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: sgautherie, Assigned: sgautherie)

Tracking

(Depends on: 1 bug)

Trunk
seamonkey2.12
Dependency tree / graph

SeaMonkey Tracking Flags

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

Details

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

Attachments

(3 attachments)

Comment 1

6 years ago
Actually all the notification event handlers should probably use capturing.
(Assignee)

Comment 2

6 years ago
Created attachment 621664 [details] [diff] [review]
(Av1) Fix phase on notification.xml handlers
[Checked in: See comments 5, 10, 15]

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)
(Assignee)

Comment 3

6 years ago
(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 4

6 years ago
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+
(Assignee)

Comment 5

6 years ago
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?
(Assignee)

Comment 6

6 years ago
(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
(Assignee)

Updated

6 years ago
Depends on: 752761

Comment 7

6 years ago
>> 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.
(Assignee)

Comment 8

6 years ago
(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+
(Assignee)

Comment 10

6 years ago
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]
(Assignee)

Updated

6 years ago
status-seamonkey2.10: affected → fixed
Keywords: checkin-needed
Whiteboard: [c-n: a0df19ed338c to c-a, comment 10 to c-b]
(Assignee)

Comment 11

6 years ago
Created attachment 625815 [details] [diff] [review]
(Bv1) Fix phase on (notification.xml) MozApplicationManifest handler
[Checked in: Comments 12 and 16]

[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?

Updated

6 years ago
Attachment #625815 - Flags: review?(neil) → review+
(Assignee)

Comment 12

6 years ago
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]
(Assignee)

Comment 13

6 years ago
Created attachment 626056 [details] [diff] [review]
(Cv1) Fix/Sync' phase on DOMMouseScroll handlers [Checked in: Comment 21]

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.

Updated

6 years ago
Attachment #625815 - Flags: approval-comm-beta?
Attachment #625815 - Flags: approval-comm-beta+
Attachment #625815 - Flags: approval-comm-aurora?
Attachment #625815 - Flags: approval-comm-aurora+
(Assignee)

Updated

6 years ago
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]
(Assignee)

Updated

6 years ago
tracking-seamonkey2.10: --- → ?
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]
(Assignee)

Updated

6 years ago
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]
(Assignee)

Updated

6 years ago
status-seamonkey2.11: affected → fixed
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+
(Assignee)

Updated

6 years ago
status-seamonkey2.12: --- → fixed
tracking-seamonkey2.10: ? → ---
tracking-seamonkey2.11: --- → ?
Target Milestone: seamonkey2.12 → seamonkey2.13

Updated

5 years ago
Target Milestone: seamonkey2.13 → ---

Updated

5 years ago
tracking-seamonkey2.11: ? → ---
Target Milestone: --- → seamonkey2.12

Comment 20

5 years ago
> 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]

Comment 21

5 years ago
> 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
Last Resolved: 5 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]

Updated

5 years ago
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]

Comment 22

5 years ago
> 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.