Closed Bug 514732 Opened 10 years ago Closed 10 years ago

Issue a DOM event for document resizing (MozScrolledAreaChanged)

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1
Tracking Status
status1.9.2 --- beta1-fixed
fennec 1.0+ ---

People

(Reporter: froystig, Assigned: froystig)

References

Details

(Keywords: dev-doc-complete, mobile)

Attachments

(5 files, 6 obsolete files)

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.
tracking-fennec: --- → ?
Attached patch Patch v1 (obsolete) — Splinter Review
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
Attachment #398884 - Flags: review?(Olli.Pettay)
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?
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.
Assignee: nobody → froystig
OS: Mac OS X → All
Hardware: x86 → All
(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.
(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.)
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.
(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?)
MozScrollAreaChanged sounds good to me.
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!
Attachment #398884 - Flags: review?(Olli.Pettay) → review-
tracking-fennec: ? → 1.0+
This blocks bug 469848 as well, since the error page has expand/collaspe sections in it.
Blocks: 469848
Attached patch Patch v2 (obsolete) — Splinter Review
Addresses Olli's comments and adds tests.
Attachment #398884 - Attachment is obsolete: true
Attachment #401394 - Flags: review?(Olli.Pettay)
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 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.
Attachment #401394 - Flags: review?(Olli.Pettay) → review+
Attached patch Patch v3Splinter Review
Address Olli's last two comments and refresh the patch.
Attachment #401394 - Attachment is obsolete: true
Attachment #402254 - Flags: review?(Olli.Pettay)
(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 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.
Attachment #402254 - Flags: review?(Olli.Pettay) → review+
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.
Attachment #402254 - Flags: superreview?(roc)
Attachment #402254 - Flags: superreview?(roc) → superreview+
pushed to m-c -- http://hg.mozilla.org/mozilla-central/rev/496ee05a3473
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: wanted1.9.2+
Resolution: --- → FIXED
Keywords: dev-doc-needed
Summary: Issue a DOM event for document resizing → Issue a DOM event for document resizing (MozScrolledAreaChanged)
Flags: in-testsuite+
Whiteboard: [doc-waiting-1.9.3]
Whiteboard: [doc-waiting-1.9.3]
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.
Whiteboard: [doc-waiting-info]
Attached patch Followup patch v1 (obsolete) — Splinter Review
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.
Attachment #408254 - Flags: review?(roc)
Attached patch Followup patch v1.1 (obsolete) — Splinter Review
Oops, accidentally left some renegade trailing whitespace in the previous patch.
Attachment #408254 - Attachment is obsolete: true
Attachment #408255 - Flags: review?(roc)
Attachment #408254 - Flags: review?(roc)
Don't create nsIHTMLScrollFrame. Just add your method to nsIScrollableFrame. Also, this needs tests.
Still waiting for info for documentation purposes as per comment #20.
Attachment #409778 - Flags: approval1.9.2?
Attachment #409778 - Flags: approval1.9.2? → approval1.9.2+
Depends on: 526051
This seems to have caused bustage on TB and SM trunk (i.e. comm-central)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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!
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Version: unspecified → Trunk
Attached patch bustage fixSplinter Review
This is my fix for the bustage.
Attachment #409829 - Flags: review?(bzbarsky)
(It actually causes bustage just on mozilla-central if you do a shared-lib build.)
Attachment #409829 - Flags: approval1.9.2?
Comment on attachment 409829 [details] [diff] [review]
bustage fix

Yay not breaking debug builds!
Attachment #409829 - Flags: review?(bzbarsky) → review+
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.
Duplicate of this bug: 526051
Attached patch bustage fix, version for 1.9.2 (obsolete) — Splinter Review
Attachment #409871 - Flags: review?(bzbarsky)
Attachment #409871 - Flags: approval1.9.2?
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?
Attachment #409871 - Flags: review?(bzbarsky) → review-
I ran with the debugger attached for a while and couldn't get it to hit this code path
finally hit this in DocShell and it works
Attachment #409871 - Attachment is obsolete: true
Attachment #409943 - Flags: review?(bzbarsky)
Attachment #409943 - Flags: approval1.9.2?
Attachment #409871 - Flags: approval1.9.2?
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?
Attachment #409943 - Flags: review?(bzbarsky) → review+
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.
Attachment #409829 - Flags: approval1.9.2? → approval1.9.2+
Attachment #409943 - Flags: approval1.9.2? → approval1.9.2+
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 on attachment 409829 [details] [diff] [review]
bustage fix

this patch was a-'d by roc on irc because of the interface change
Attachment #409829 - Flags: approval1.9.2+
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. :)
Whiteboard: [doc-waiting-info]
Attached patch Tests for followup patch (obsolete) — Splinter Review
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.
Attachment #418043 - Flags: review?(bzbarsky)
Attachment #418043 - Flags: review?(bzbarsky) → review?(Olli.Pettay)
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
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.
Attachment #418043 - Attachment is obsolete: true
Attachment #418198 - Flags: review?(Olli.Pettay)
Attachment #418043 - Flags: review?(Olli.Pettay)
Comment on attachment 418198 [details] [diff] [review]
Tests for followup patch

I think this is ok.
Attachment #418198 - Flags: review?(Olli.Pettay) → review+
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/.
Keywords: mobile
You need to log in before you can comment on or make changes to this bug.