514732 - Issue a DOM event for document resizing (MozScrolledAreaChanged)

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Roy Frostig :froystig]

Fennec frontend would like to have a way of receiving asynchronous notifications about changes to a document body's scrollWidth and scrollHeight.  These can usually be retreived in JS via |document.body.scrollWidth| and |.scrollHeight|, but accessing these values may cause reflow, which is especially expensive at page load time, during which these values change often.  Instead of polling these values with potential performance expense, it would be ideal to have a DOM event (similar to type "MozAfterPaint") which provides these values whenever they change.

Roc recommends watching the values of the overflow area of a root |nsHTMLScrollFrame| at reflow time and posting the event at that point.

Smaug recommends using the already existing but unused NS_SCROLLPORT_EVENT with the NS_SCROLLPORT_OVERFLOWCHANGED message as the internal nsGUIEvent to fire.

I've got a v1 patch coming soon.

Comment 1

[Roy Frostig :froystig]

Attachment: Patch v1

Here's a patch in which everything seems to be working correctly.  Be careful of two fprintf() calls I've left in |nsHTMLScrollFrame::Reflow| which help to see how often the event is issued.

Johnny Stenback has mentioned that this event sounds like one that content authors have been requesting for some time --- an event notifying that the overflow area of any arbitrary element has changed.  I've tried to make this |nsIDOMOverflowAreaEvent| fit these general needs, so that it can be used for that future purpose as well.  I'm not sure where such an event would be posted from within the layout engine, but I'd be happy to help make work once this is approved.  Currently, this event should only get posted by the reflow of the root nsHTMLScrollFrame, as per roc's suggestion.


I added a few lines of a JS listener to my Fennec build that simply displays the received event data.  Here's some sample (exclamatory) listener output:

!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
Got overflowchanged event!
  type: overflowchanged
  x, y, width, height: 0 0 800 500
  view: [object XPCNativeWrapper [object Window @ 0xb0d18140 (native @ 0xb79b9f30)]]
  detail: 0
  target: [object XPCNativeWrapper [object HTMLDocument @ 0xb0774bc0 (native @ 0xb0dc7c00)]]
    target.location (?): file:///home/froystig/dynamic-expand-test.html
  currentTarget: [object XULElement @ 0xb0d59200 (native @ 0xb0d196d0)]
  eventPhase: 3
  bubbles: true
  cancelable: true
  timeStamp: 0

!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
Got overflowchanged event!
  type: overflowchanged
  x, y, width, height: 0 0 3500 3333
  view: [object XPCNativeWrapper [object Window @ 0xb0d18140 (native @ 0xb79b9f30)]]
  detail: 0
  target: [object XPCNativeWrapper [object HTMLDocument @ 0xb0774bc0 (native @ 0xb0dc7c00)]]
    target.location (?): file:///home/froystig/dynamic-expand-test.html
  currentTarget: [object XULElement @ 0xb0d59200 (native @ 0xb0d196d0)]
  eventPhase: 3
  bubbles: true
  cancelable: true
  timeStamp: 0

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Should we call this something like "scrollareachanged" or "scrolledareachanged" instead?  The reason I'm thinking that is that all elements have an overflow area, but only when the CSS property 'overflow' has a non-default value is the element scrollable, and it's only for the latter elements (those that have a scroll frame) that you're firing this event.

The other interesting question is if the overflow area changes in a way that doesn't affect the scrollable area, should you fire the event?  For example, in LTR documents, overflow to the top or left doesn't change what's scrollable since we don't scroll to it -- are you firing events for those cases?  Do you want to be?

Comment 3

[Roy Frostig :froystig]

On the topic of naming, I am not insisting at all that the names I've chosen in the patch are good, and what you suggest is likely better.  I'm not entirely familiar with all of layout terminology.  Do keep in mind Johnny's request for using this same DOM event in the future to mean "the element has resized".  I wouldn't mind firing this elsewhere to better fit the general criteria, but I'd need some advice in knowing where to do so, and would still like it to work in the case when we want to know whether or not the document resized.  If what this bug requires is actually completely different from the general case, then let's settle on a name different enough, and leave the other DOM event to be taken care of later.

Regarding your second question, I am firing the event whenever the value of |mInner.mScrolledFrame->GetView()->GetBounds()| changes during the reflow of a root |nsHTMLScrollFrame|.  I would guess that this predicate is true if and only if the overflow of the scrolled area changes (despite that it may not affect the scrollable area), but I could be wrong.  You have a good point in asking whether or not this is the desired firing criteria, which makes me wonder if this is even a matter relevant necessarily to the scrollframe at all, or if the event dispatch should ideally sit elsewhere altogether.  I would need some advice there as well, but I do know that the goals are: 1) To replace Fennec's need to poll the value of |document.body.scrollWidth| and |.scrollHeight|, and 2) To behave in a way similar to other browsers that provide content JS with DOM event notifications of resizing elements.  This bug is specific to solving the former, but if we can have both then I'd be happy to have that done here.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #3)
> familiar with all of layout terminology.  Do keep in mind Johnny's request for
> using this same DOM event in the future to mean "the element has resized".

For "the element has resized", we should be using onresize, which IE already implements for all elements, and we should do the same.  But when the scrollable area of an element changes is different from when it resizes.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #4)
> For "the element has resized", we should be using onresize, which IE already
> implements for all elements, and we should do the same.  But when the
> scrollable area of an element changes is different from when it resizes.

(Doing that is bug 227495.)

Comment 6

[Roy Frostig :froystig]

Okay, so I understand that these two tasks aren't as tightly coupled as some had hoped for.  Probably the best I can do is rename the DOM event's interface (nsIDOMOverflowAreaEvent in my current patch) to something that sounds more like it suits both situations (since the data that the event carries already does).  Whoever fixes bug 227495 some day can feel free to use the same event interface.

I need to figure out the following.  If anyone already knows the answers, I'll be happy to hear them.

* Do the values of |document.body.scrollWidth| and |document.body.scrollHeight| (in JS) correspond to the scrolled area, or just the scrollable area?  My guess is the latter.  The answer to this one will likely determine when I want to fire the event.

* Does the value of |mInner.mScrolledFrame->GetView()->GetBounds()| in nsHTMLScrollFrame::Reflow() correspond to the scrolled area, or just the scrollable area?  My guess is the former.  The answer to this one will likely determine whether I need to change that part of my patch.

With that figured out, I'll take dbaron's opinion on the exposed event type name.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #6)
> Okay, so I understand that these two tasks aren't as tightly coupled as some
> had hoped for.  Probably the best I can do is rename the DOM event's interface
> (nsIDOMOverflowAreaEvent in my current patch) to something that sounds more
> like it suits both situations (since the data that the event carries already
> does).  Whoever fixes bug 227495 some day can feel free to use the same event
> interface.

I don't think we need any additional infrastructure in the event code for onresize, since we already implement onresize for some elements, just not all of them.  (But is there something in our existing onresize implementation that makes sense for you to share?)

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

MozScrollAreaChanged sounds good to me.

Comment 9

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

Comment on attachment 398884 [details] [diff] [review]
Patch v1

>+    { &nsGkAtoms::onoverflowchanged,             { NS_SCROLLPORT_OVERFLOWCHANGED, EventNameType_HTMLXUL }},
So apparently the event name should be changed to MozScrollAreaChanged, 
in which case you could perhaps create a new NS_ event type and not reuse
NS_SCROLLPORT_OVERFLOWCHANGED.

>+nsDOMOverflowAreaEvent::nsDOMOverflowAreaEvent(nsPresContext *aPresContext,
>+                                               nsScrollPortEvent *aEvent)
>+  : nsDOMUIEvent(aPresContext, aEvent)
>+{
>+  mClientOverflowArea.SetLayoutRect(aEvent->mOverflowArea, aPresContext);
>+}

This crashes if someone calls document.createElement("overflowchanged");

>@@ -717,11 +719,13 @@ nsEventDispatcher::CreateEvent(nsPresCon
>   if (aEventType.LowerCaseEqualsLiteral("notifypaintevent"))
>     return NS_NewDOMNotifyPaintEvent(aDOMEvent, aPresContext, nsnull);
>   if (aEventType.LowerCaseEqualsLiteral("simplegestureevent"))
>     return NS_NewDOMSimpleGestureEvent(aDOMEvent, aPresContext, nsnull);
>   if (aEventType.LowerCaseEqualsLiteral("beforeunloadevent"))
>     return NS_NewDOMBeforeUnloadEvent(aDOMEvent, aPresContext, nsnull);
>   if (aEventType.LowerCaseEqualsLiteral("pagetransition"))
>     return NS_NewDOMPageTransitionEvent(aDOMEvent, aPresContext, nsnull);
>+  if (aEventType.LowerCaseEqualsLiteral("overflowchanged"))
>+    return NS_NewDOMOverflowAreaEvent(aDOMEvent, aPresContext, nsnull);
You should probably use something else as the type.
Maybe "scrollareaevent"?

>+  DOM_CLASSINFO_MAP_BEGIN(OverflowAreaEvent, nsIDOMOverflowAreaEvent)
>+    DOM_CLASSINFO_MAP_ENTRY(nsIDOMOverflowAreaEvent)
>+    DOM_CLASSINFO_EVENT_MAP_ENTRIES    
>+  DOM_CLASSINFO_MAP_END
Not DOM_CLASSINFO_EVENT_MAP_ENTRIES, but DOM_CLASSINFO_UI_EVENT_MAP_ENTRIES.

>+nsHTMLScrollFrame::PostScrolledAreaEvent(nsRect &aScrolledArea)
>+{
>+  if (mScrolledAreaEventDispatcher.IsPending()) {
>+    mScrolledAreaEventDispatcher.get()->mScrolledArea = aScrolledArea;
>+    return;
>+  }
>+
>+  ScrolledAreaEventDispatcher *dp = new ScrolledAreaEventDispatcher(this);
>+  dp->mScrolledArea = aScrolledArea;
This crashes if OOM

>+  if (NS_FAILED(NS_DispatchToCurrentThread(dp))) {
>+    NS_WARNING("failed to dispatch ScrolledAreaEventDispatcher");
This leaks dp. Better to make the variable
nsRefPtr<ScrolledAreaEventDispatcher> dp.

Tests!

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

This blocks bug 469848 as well, since the error page has expand/collaspe sections in it.

Comment 11

[Roy Frostig :froystig]

Attachment: Patch v2

Addresses Olli's comments and adds tests.

Comment 12

[Roy Frostig :froystig]

Just came to my attention that my mochitests in this patch are showing a leaking of nsRect instances.  Will try to take a look at that tomorrow evening.  Olli, feel free to postpone review until I have that fixed.

Comment 13

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

Comment on attachment 401394 [details] [diff] [review]
Patch v2

Please use -U 8 -p when creating patches!

>+nsDOMScrollAreaEvent::nsDOMScrollAreaEvent(nsPresContext *aPresContext,
>+                                           nsScrollAreaEvent *aEvent)
>+  : nsDOMUIEvent(aPresContext, aEvent)
>+{
>+  mClientArea.SetLayoutRect(aEvent ? aEvent->mArea : nsRect(), aPresContext);
>+}
>+

>+nsDOMScrollAreaEvent::InitScrollAreaEvent(const nsAString &aEventType,
>+                                          PRBool aCanBubble,
>+                                          PRBool aCancelable,
>+                                          nsIDOMAbstractView *aView,
>+                                          PRInt32 aDetail,
>+                                          float x, float y,
>+                                          float width, float height)
Nit, all the parameters should be in form 'aParameter'

>+void
>+nsHTMLScrollFrame::FireScrolledAreaEvent(nsRect &aScrolledArea)
>+{
>+  mScrolledAreaEventDispatcher.Forget();
>+
>+  nsScrollAreaEvent event(PR_TRUE, NS_SCROLLEDAREACHANGED, nsnull);
>+  nsEventStatus status = nsEventStatus_eIgnore;
>+  nsPresContext *prescontext = PresContext();
>+  nsIContent* content = GetContent();
>+
>+  event.mArea = aScrolledArea;
>+
>+  nsIDocument *doc = content->GetCurrentDoc();
>+  if (doc) {
>+    nsEventDispatcher::Dispatch(doc, prescontext, &event, nsnull, &status);
The last parameter is optional, and since you're not using status for anything,
no need to leave it there.

Have you tested how often do we dispatch these events?
If *very* often, we may need to add some optimization to not dispatch them
if there are no listeners for them - at least add the optimization if this affects tp/tdhtml. Can be done in a separate bug if needed.

Comment 14

[Roy Frostig :froystig]

Attachment: Patch v3

Address Olli's last two comments and refresh the patch.

Comment 15

[Roy Frostig :froystig]

(In reply to comment #13)
> (From update of attachment 401394 [details] [diff] [review])
> Please use -U 8 -p when creating patches!

I'm not entirely sure what these parameters do, but I changed my config to include these, so I hope the latest patch has that.  Let me know if it doesn't and I'll be happy to repost.

> Have you tested how often do we dispatch these events?
> If *very* often, we may need to add some optimization to not dispatch them
> if there are no listeners for them - at least add the optimization if this
> affects tp/tdhtml. Can be done in a separate bug if needed.

Some basic testing shows that the events are not dispatched any more than expected (after all, it happens *at most* with every reflow of a root nsHTMLScrollFrame and I take it we try to minimize those).  On most static pages, firing amount is proportional to the number of root scroll frames, so for instance, cnn.com causes quite a few (7-12) of these to fire as the page is loading.

Comment 16

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

Comment on attachment 402254 [details] [diff] [review]
Patch v3

Might be good to post to tryserver to see if this causes tp regression.
I guess it doesn't.

Comment 17

[Roy Frostig :froystig]

When I posted to try all tp was green.  There were two seemingly very unrelated timeouts during unit tests, one of which I was able to recognize as a known issue.

Comment 18

[Stuart Parmenter]

pushed to m-c -- http://hg.mozilla.org/mozilla-central/rev/496ee05a3473

Comment 19

[Doug Turner (:dougt)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/aecf09956fc5

Comment 20

[Eric Shepherd [:sheppy]]

There aren't any useful comments in the IDL for nsIDOMScrollAreaEvent. There should be some. Looking at the patch, I can't figure out with any certainty what any of the attributes on this interface mean, so I can't really document this. Would appreciate some info.

Comment 21

[Roy Frostig :froystig]

Attachment: Followup patch v1

Found out that the Fennec frontend wanted this event to fire when a page is restored from history as well as upon reflow.  Earlier (already checked-in) patch posts the event as the result of reflow, and this patch posts it from nsDocShell::RestoreFromHistory(), in the case where restoring from history could mean no reflow will occur.

Comment 22

[Roy Frostig :froystig]

Attachment: Followup patch v1.1

Oops, accidentally left some renegade trailing whitespace in the previous patch.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Don't create nsIHTMLScrollFrame. Just add your method to nsIScrollableFrame. Also, this needs tests.

Comment 24

[Eric Shepherd [:sheppy]]

Still waiting for info for documentation purposes as per comment #20.

Comment 25

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: followup patch v1.2, don't create nsIHTMLScrollFrame

Comment 26

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mozilla-central/rev/4c63a655eac6

Comment 27

[Ian Neal]

This seems to have caused bustage on TB and SM trunk (i.e. comm-central)

Comment 28

[Serge Gautherie (:sgautherie)]

(mid-air collision...)

(In reply to comment #26)
> pushed http://hg.mozilla.org/mozilla-central/rev/4c63a655eac6

In future, _please_ add bug number to changeset and file a c-c bug (like bug 526051) to avoid busting other trees!

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: bustage fix

This is my fix for the bustage.

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(It actually causes bustage just on mozilla-central if you do a shared-lib build.)

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 409829 [details] [diff] [review]
bustage fix

Yay not breaking debug builds!

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Unfortunately this bustage fix can't be applied to the branch as-is, because it modifies nsIPresShell, which is used by some binary extensions.

One way to deal with this is to make a new interface nsIPresShell_MOZILLA_1_9_2 which PresShell QIs to, add GetRootScrollFrameAsScrollableExternal to that interface, and use that from docshell. On the branch only, of course.

Comment 34

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: bustage fix, version for 1.9.2

Comment 35

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 409871 [details] [diff] [review]
bustage fix, version for 1.9.2

This doesn't change the presshell QI impl, so presumably doesn't work right.  Was it tested?

Comment 36

[Brad Lassey [:blassey] (use needinfo?)]

I ran with the debugger attached for a while and couldn't get it to hit this code path

Comment 37

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: bustage fix, version 2 for 1.9.2

finally hit this in DocShell and it works

Comment 38

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 409943 [details] [diff] [review]
bustage fix, version 2 for 1.9.2

Can we get some automated tests here, at least as a followup?

Comment 39

[Boris Zbarsky [:bzbarsky]]

And specifically, see http://mxr.mozilla.org/mozilla-central/source/docshell/test/chrome/docshell_helpers.js and its consumers for some examples of how to write such a test.

Comment 40

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 409943 [details] [diff] [review]
bustage fix, version 2 for 1.9.2

a192=beltzner, I like me some bustage fixes; tests?

Comment 41

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/79e0584f3687 and http://hg.mozilla.org/releases/mozilla-1.9.2/rev/e6b7925d7fc1

Comment 42

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 409829 [details] [diff] [review]
bustage fix

this patch was a-'d by roc on irc because of the interface change

Comment 43

[Eric Shepherd [:sheppy]]

I've documented this here:

https://developer.mozilla.org/en/DOM/Detecting_document_width_and_height_changes

Is this really in 1.9.2? It looks like it but is hard to tell; some of the comments are contradictory on this point. :)

Comment 44

[Roy Frostig :froystig]

Attachment: Tests for followup patch

These test the RestoreFromHistory() path for event firing introduced in the earlier followup patch (attachment 409778 [details] [diff] [review]).

Added these to layout/generic/test/ because the rest of the earlier tests are in that directory as well.

Comment 45

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

Comment on attachment 418043 [details] [diff] [review]
Tests for followup patch


>+      doPageNavigation( {
>+        back: true,
>+        eventsToListenFor: ["MozScrolledAreaChanged"],
>+        expectedEvents: [ { type: "MozScrolledAreaChanged",
>+                            x: 0, y: 0, width: 10500, height: 10500 } ],
>+        onNavComplete: nextTest

Can doPageNavigation really handle x/y/width//height?

If it can, could you fix the documentation in 
http://mxr.mozilla.org/mozilla-central/source/docshell/test/chrome/docshell_helpers.js#25

Comment 46

[Roy Frostig :froystig]

Attachment: Tests for followup patch

They do not handle it actually, those properties are ignored.  I had that in there because I was trying to see if they'd be noticed somehow, and then forgot to remove it.  Removed that in this updated patch.  Let me know if you know of any straightforward way to check the event properties using the doPageNavigation harness.  If not, it should be OK still since I test that plenty often in the old tests.

Comment 47

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

Comment on attachment 418198 [details] [diff] [review]
Tests for followup patch

I think this is ok.

Comment 48

[Joel Maher ( :jmaher ) (UTC -8)]

do we know why this is located in layout/generic/test (as the only test case), but relies one the majority of the functionality in docshell/test/chrome/docshell_helpers.js?  I would think this should live in dochsell/test/chrome/.

Comment 49

[Robert O'Callahan (:roc) (email my personal email if necessary)]

that's fine.