416226 - Need specific event to notify fullZoom changes

Status: NEW

(Core :: DOM: Events, defect, P5)

Description

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

If an extension to FF3 changes the fullZoom factor directly through the markupDocumentViewer, the zoom factor in the UI (and in other codes/extensions dealing with it) is not updated because we have no way to notify these codes the fullZoom factor changed. I suggest a specific event to do that, dispatched when |nsIMarkupDocumentViewer.SetFullZoom()| succeeds.

Comment 1

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

taking bug

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

This is related to bug 303405.

Comment 3

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: fix #1

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 302576 [details] [diff] [review]
fix #1

>+  nsEvent evt(PR_TRUE, NS_FULLZOOMCHANGED);
>+  // XXX Dispatching to |window|, but using |document| as the target.
>+  evt.target = mDocument;
We should limit this hack, hopefully to load/unload events which can't be changed without
breaking lots of sites.

>+    { &nsGkAtoms::onfullzoomchanged,             { NS_FULLZOOMCHANGED, EventNameType_All }},
Do we need to add support for the new attribute? Would it be enough to be able to use
window.addEventListener("FullZoomChanged", ...);

If yes, then it is enough to just call
nsContentUtils::DispatchTrustedEvent(mDocument, mDocument->GetWindow(), "FullZoomChanged", PR_TRUE, PR_TRUE, nsnull);
in nsDocumentViewer.cpp. No need to create new nsEvent type or anything.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

And perhaps the event shouldn't bubble. That way event listeners (in chrome)
for bubble phase wouldn't get called when subwindows get zoomed.
But if someone really wants to catch also those events, using capturing
listener would allow that.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Still one comment. I'd say it is better to dispatch the event after viewmanager update batch. Usually better to avoid running scripts while in a vm batch (though that is safe).

Comment 7

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: fix #2 in answer to smaug

Comment 8

[Boris Zbarsky [:bzbarsky]]

So my general comments:

1)  I don't like adding yet another completely undocumented event.  We need to have some documentation to go with this (devmo?).
2)  I don't like adding yet another non-gecko-namespaced event, though perhaps it's OK.
3)  Executing script in the middle of CallChildren might or might not be a good idea.  I guess the call on the prescontext might do that anyway, so this stuff might already be prone to crashes and exploits (since we hold no strong ref to the prescontext or presshell here).... But making it easier to hook in evil code seems like a dubious thing security-wise.

Comment 9

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

(In reply to comment #8)
> So my general comments:
> 
> 1)  I don't like adding yet another completely undocumented event.  We need to
> have some documentation to go with this (devmo?).

Sure. No checkin about this w/o doc.

> 3)  Executing script in the middle of CallChildren might or might not be a good
> idea.  I guess the call on the prescontext might do that anyway, so this stuff
> might already be prone to crashes and exploits (since we hold no strong ref to
> the prescontext or presshell here).... But making it easier to hook in evil
> code seems like a dubious thing security-wise.

I chose this way of doing because it was really unintrusive in terms of existing code. We can of course dispatch the event after the completion of CallChildren, and that will solve the multiple-event-per-window problem.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

The event could be called MozFullZoomChanged and it should be sent asynchronously
or using a script runner.