Last Comment Bug 339807 - SVG layout changes should be flushed before DOM calls
: SVG layout changes should be flushed before DOM calls
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Robert Longson
: Hixie (not reading bugmail)
: Jet Villegas (:jet)
Mentors:
http://www.carto.net/neumann/mozillas...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-05-31 06:21 PDT by Robert Longson
Modified: 2006-06-13 03:20 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
flush layout changes before calling DOM functions (6.65 KB, patch)
2006-05-31 06:25 PDT, Robert Longson
no flags Details | Diff | Splinter Review
address bz's comments (8.90 KB, patch)
2006-05-31 08:55 PDT, Robert Longson
tor: review+
bzbarsky: superreview+
Details | Diff | Splinter Review

Description Robert Longson 2006-05-31 06:21:25 PDT
If the layout is updated and DOM functions are called the functions will return data appropriate to the old rather than the updated layout.
Comment 1 Robert Longson 2006-05-31 06:25:17 PDT
Created attachment 223918 [details] [diff] [review]
flush layout changes before calling DOM functions

As discussed in mozilla.tech.dev.svg group

Bug .getBBox() and .getCharNumAtPosition() after modifiying text-anchor attribute
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2006-05-31 08:37:42 PDT
> +    NS_ERROR("no document");

Er... NO. Do NOT do that.  Having no current doc is a perfectly reasonable state of things for a DOM node.  Having a DOM API called on such node is also perfectly reasonable.  Asserting when this happens is NOT acceptable.  Furthermore, does the SVG spec really not define a better error code than NS_ERROR_FAILURE for this case?

> +    NS_ERROR("no presshell");

Again, this is NOT something you can assert.

I see that nsSVGPathElement, nsSVGTextElement, nsSVGTextPathElement, and nsSVGTSpanElement already have such bogus asserts.  Please remove them.
Comment 3 Robert Longson 2006-05-31 08:55:43 PDT
Created attachment 223933 [details] [diff] [review]
address bz's comments
Comment 4 Robert Longson 2006-05-31 08:59:01 PDT
As far as I can tell the SVG spec does not define any failure codes except invalid argument values for it's own functions. Things like what happens if the element is display:none and you call a DOM function on it are not mentioned.
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2006-06-02 08:36:04 PDT
Comment on attachment 223933 [details] [diff] [review]
address bz's comments

sr=bzbarsky, though this looks like it should perhaps be a GetPresShell method on some common superclass instead of copy/pasting the same code a bunch of times.
Comment 6 Robert Longson 2006-06-06 02:27:28 PDT
Checked in.

Checking in nsSVGGraphicElement.cpp;
/cvsroot/mozilla/content/svg/content/src/nsSVGGraphicElement.cpp,v  <--  nsSVGGraphicElement.cpp
new revision: 1.32; previous revision: 1.31
done
Checking in nsSVGPathElement.cpp;
/cvsroot/mozilla/content/svg/content/src/nsSVGPathElement.cpp,v  <--  nsSVGPathElement.cpp
new revision: 1.18; previous revision: 1.17
done
Checking in nsSVGSVGElement.cpp;
/cvsroot/mozilla/content/svg/content/src/nsSVGSVGElement.cpp,v  <--  nsSVGSVGElement.cpp
new revision: 1.69; previous revision: 1.68
done
Checking in nsSVGTSpanElement.cpp;
/cvsroot/mozilla/content/svg/content/src/nsSVGTSpanElement.cpp,v  <--  nsSVGTSpanElement.cpp
new revision: 1.22; previous revision: 1.21
done
Checking in nsSVGTextElement.cpp;
/cvsroot/mozilla/content/svg/content/src/nsSVGTextElement.cpp,v  <--  nsSVGTextElement.cpp
new revision: 1.25; previous revision: 1.24
done
Checking in nsSVGTextPathElement.cpp;
/cvsroot/mozilla/content/svg/content/src/nsSVGTextPathElement.cpp,v  <--  nsSVGTextPathElement.cpp
new revision: 1.6; previous revision: 1.5
done

Note You need to log in before you can comment on or make changes to this bug.