Bug 16034 - REGRESSION (r27351): Departure date does not repaint when changed on Google Maps public transit planner
Summary: REGRESSION (r27351): Departure date does not repaint when changed on Google M...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.4
: P1 Normal
Assignee: Nobody
URL: http://www.google.com/maps?ie=UTF8&f=...
Keywords: InRadar, Regression
Depends on:
Blocks:
 
Reported: 2007-11-17 17:17 PST by mitz
Modified: 2007-11-21 21:24 PST (History)
1 user (show)

See Also:


Attachments
Track the layout root using the render tree and the container() relation (22.95 KB, patch)
2007-11-18 18:57 PST, mitz
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2007-11-17 17:17:09 PST
To reproduce this bug:
1) Open the URL
2) Click the Options tab
3) Click the calendar icon next to the date
4) Click a different date

Notice that the value in the text field does not change. Forcing a repaint changes it. Works with shipping Safari 3.0.4 on Leopard.
Comment 1 mitz 2007-11-17 17:19:00 PST
<rdar://problem/5607037>
Comment 2 mitz 2007-11-17 18:19:58 PST
Regressed in r27351 (subtree layout for overflow areas). Just commenting-out the extra condition in objectIsRelayoutBoundary() fixes the bug.
Comment 3 mitz 2007-11-17 23:42:18 PST
The root cause for the bug is that the code in FrameView::scheduleRelayoutOfSubtree() to handle a new root that is an ancestor or a descendant of the current root is wrong. It uses the DOM tree while in fact it should use the container()-hood graph of render objects. In the Google Maps case, the result is that when scheduleRelayoutOfSubtree() tries to mark the path from the descendant up to the ancestor, it marks all the way up to the root. Consequently, at the end of the subtree layout, the root is still marked for layout, and a second layout happens under invalidateSelection(), exposing a different bug: the inner layout clears the repaint rects set held by the frame, so the repaint never happens.
Comment 4 mitz 2007-11-18 18:57:49 PST
Created attachment 17372 [details]
Track the layout root using the render tree and the container() relation

In addition to fixing the root cause and the other bug mentioned in comment #3, this patch changes layoutRoot from a RefPtr<Node> to a RenderObject*. It makes more sense to keep track of the layout root in terms of the render tree, and the patch includes an ASSERT that the layout root does not get deleted.

No layout test regressions.
Comment 5 Eric Seidel (no email) 2007-11-21 20:58:42 PST
Comment on attachment 17372 [details]
Track the layout root using the render tree and the container() relation

You might add a comment to:
RenderObject* layoutRoot;
something like:
// weak reference, ASSERT in ~RenderObject assures this is never made stale

I need to ask you a couple questions over IRC.
Comment 6 Eric Seidel (no email) 2007-11-21 21:14:56 PST
Comment on attachment 17372 [details]
Track the layout root using the render tree and the container() relation

Looks good. r=me.
Comment 7 mitz 2007-11-21 21:24:07 PST
Fixed in <http://trac.webkit.org/projects/webkit/changeset/27952>.