User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release) Build ID: 20140306171728 Steps to reproduce: 1) Unzip attached HTML file 2) Edit file to uncomment the style tag and the correct filter_param code block for the browser you want to test with. 3) Open the browser and make sure that the dev tools console is showing. 4) Load up the file in the browser. 5) 5s after opening a script will run to walk the DOM for a certain set of nodes and get the bounding rects for the nodes. It will report the total time this took when completed. Actual results: Time to complete DOM walk: For FireFox Without Multicolumn: ~200ms With Multicolumn: ~30s For Chrome Without Multicolumn: ~30ms With Multicolumn: ~30ms For IE Without Multicolumn: ~130ms With Multicolumn: ~130ms Expected results: Time for walking the DOM and getting bounding rects should be approximately equivalent. I'd be OK with an order of magnitude but 2+ orders of magnitude is just horrible (might I add embarrassing)?

Based on profile this is all layout/reflow.

(I don't understand why we end up reflowing so many times.)

This is pretty ridiculous. What happens here is that nsRange::GetBoundingClientRect calls CollectClientRects, which calls GetPartialTextRect, which calls GetTextFrameForContent, which calls nsCSSFrameConstructor::EnsureFrameForTextNode. In cases when the textnode had a frame suppressed (i.e. it was whitespace-only) this causes us to create and insert the new frame and then we have to do layout. In the columns case layout is a lot slower than in the non-columns case to do height-balancing (which Chrome doesn't do particularly well, by the way). This issue will only affect people asking for getBoundingClientRect on collapsed textnodes... If I comment out this line of the testcase: bounding_rect = range.getBoundingClientRect(); then I get times like so: For FireFox Without Multicolumn: ~24ms With Multicolumn: ~24ms For Chrome Without Multicolumn: ~21ms With Multicolumn: ~21ms Robert, is there any way we can just avoid creating the frames in this situation?

Chrome deals with this by just being incorrect. Some collapsed-away text nodes (but not all) do not contribute to the list returned by getClientRects (or, presumably, getBoundingClientRect). See this testcase, in which the first box should always be zero-sized. Also, there should be three boxes in each list. Chrome returns only 1 in the second case, which is inexplicable.

In fact, even in the Joseph's testcase, Chrome is returning incorrect results for all the collapsed-away text nodes

(It's returning 0,0,0,0 when it should be returning reasonable x/y values.)

Hmm. IE11 also seems to return only 1 rect, if I'm testing right...

The easiest way to fix this case is to have a document-wide flag to disable the collapsed-away text node optimization when we discover we need the text frame for any collapsed-away node.

(In reply to Boris Zbarsky [:bz] from comment #7) > Hmm. IE11 also seems to return only 1 rect, if I'm testing right... In which part of the test? What does IE11 do for the second part of the test?

For the first part of the test, IE has rects.length == 0. For the second part of the test, IE has rects.length == 1, and the rect has x=9, y=271.4, width=0, height=0.

This fix reveals a columns bug with the reporter's testcase. Filed as bug 983581.

Comment on attachment 8391109 [details] [diff] [review] fix > if (frame && frame->GetType() == nsGkAtoms::textFrame) { >+ aContent->OwnerDoc()->FlushPendingNotifications(Flush_Layout); > return static_cast<nsTextFrame*>(frame); frame maybe a dead object here.

Comment on attachment 8391109 [details] [diff] [review] fix Olli is right. We should either flush before doing the GetPrimaryFrame() call in EnsureFrameForTextNode or reget the primary frame after flushing. And we should document the mAlwaysCreateFramesForIgnorableWhitespace member to make it clear that we turn off the optimization globally because if people are poking at whitespace sizes/positions they'll probably keep doing it. Or something like that. r=me with that

(In reply to Boris Zbarsky [:bz] from comment #14) > Olli is right. We should either flush before doing the GetPrimaryFrame() > call in EnsureFrameForTextNode or reget the primary frame after flushing. Good point. I'll do the latter. > And we should document the mAlwaysCreateFramesForIgnorableWhitespace member > to make it clear that we turn off the optimization globally because if > people are poking at whitespace sizes/positions they'll probably keep doing > it. Or something like that. Sure.

(In reply to Boris Zbarsky [:bz] from comment #3) > This is pretty ridiculous. > > What happens here is that nsRange::GetBoundingClientRect calls > CollectClientRects, which calls GetPartialTextRect, which calls > GetTextFrameForContent, which calls > nsCSSFrameConstructor::EnsureFrameForTextNode. > > In cases when the textnode had a frame suppressed (i.e. it was > whitespace-only) this causes us to create and insert the new frame and then > we have to do layout. In the columns case layout is a lot slower than in > the non-columns case to do height-balancing (which Chrome doesn't do > particularly well, by the way). > > This issue will only affect people asking for getBoundingClientRect on > collapsed textnodes... If I comment out this line of the testcase: > > bounding_rect = range.getBoundingClientRect(); > > then I get times like so: > > For FireFox > Without Multicolumn: ~24ms > With Multicolumn: ~24ms > > For Chrome > Without Multicolumn: ~21ms > With Multicolumn: ~21ms > > Robert, is there any way we can just avoid creating the frames in this > situation? Is there a way to tell if a text nod eis collapsed on the client side?

Not easily. It depends on all sorts of stuff. A prerequisite is it being whitespace-only (and even that doesn't have a nice API to detect it), but various other conditions apply too.

Verified on Mac OSX 10.8.5 using Firefox 31.0b8 and the following times are achieved: Without Multicolumn: ~88ms With Multicolumn: ~120ms - If I comment out this line of the testcase: bounding_rect = range.getBoundingClientRect(); then I get times like so: Without Multicolumn: ~27ms With Multicolumn: ~27ms