The default bug view has changed. See this FAQ.

viewport changes should only affect transforms if we have a viewBox attribute

RESOLVED FIXED in mozilla15

Status

()

Core
SVG
RESOLVED FIXED
9 years ago
5 years ago

People

(Reporter: Robert Longson, Assigned: jwatt)

Tracking

({perf})

Trunk
mozilla15
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

9 years ago
Follow-up from bug 411555 comment 26.
(Assignee)

Updated

9 years ago
Depends on: 374111
(Reporter)

Comment 1

7 years ago
In nsSVGSVGElement::GetViewBoxTransform if the viewPortWidth or viewportHeight is zero then the transform returned is singular, otherwise it is an identity matrix. So in actual fact the transform can change.

So to fix this we'd have to detect when the svg width/height changed from 0 to non-zero and vice versa. Is that worth doing Jonathan?
(Assignee)

Comment 2

5 years ago
This is unnecessarily hurting us during window/viewport resize of expensive SVGs that do not actually have a valid viewBox rect. We have at least one bug open that would benefit from this being fixed, although I can't currently remember where I saw it.
Assignee: nobody → jwatt
Keywords: perf
(Assignee)

Comment 3

5 years ago
Created attachment 620679 [details] [diff] [review]
Clean up and document misleading viewBox code
Attachment #620679 - Flags: review?(longsonr)
(Assignee)

Comment 4

5 years ago
Created attachment 620680 [details] [diff] [review]
Only send the modification flags that are needed
Attachment #620680 - Flags: review?(longsonr)
(Reporter)

Comment 5

5 years ago
Comment on attachment 620679 [details] [diff] [review]
Clean up and document misleading viewBox code

>+  /**
>+   * Returns true if this element has a base/anim value for its "viewBox"
>+   * attribute that defines a viewBox rectangle with finite values.
>+   *
>+   * Note that this does not check whether we need to synthesize a viewBox,
>+   * so you must call ShouldSynthesizeViewBox() if you need to check that too.
>+   *
>+   * Note also that this method does not pay attention to whether the width or
>+   * height values of the viewBox rect are positive!
>+   */
>+  bool HasViewBoxRect() const {
>+    return mViewBox.HasRect();
>+  }

Don't really see the point of this. Callers can just call mViewBox.HasRect() directly.

> 
> class nsSVGViewBox
> {
> 
>+  /**
>+   * Returns true if the corresponding "viewBox" attribute defined a rectangle
>+   * with finite values. Returns false if the viewBox was set to "none" or an
>+   * invalid string, or if any of the four rect values were too big to store in
>+   * a float.
>+   *
>+   * This method does not check whether the width or height values are positive,
>+   * so callers must check whether the viewBox rect is valid if necessary!
>+   */
>+  bool HasRect() const
>     { return (mHasBaseVal || mAnimVal); }

Every other type uses IsExplicitlySet so that's what viewBox should use rather than HasRect or IsValid.
Attachment #620679 - Flags: review?(longsonr) → review-
(Assignee)

Comment 6

5 years ago
(In reply to Robert Longson from comment #5)
> Don't really see the point of this. Callers can just call mViewBox.HasRect()
> directly.

It's cleaner not to have other classes reaching into private members IMO.

> Every other type uses IsExplicitlySet so that's what viewBox should use
> rather than HasRect or IsValid.

I disagree in this case, since IsExplicitlySet would sound like it should return true for viewBox="none", when actually what the callers are interested in is whether there is a rect defined.
(Reporter)

Comment 7

5 years ago
Does part 2 pass try? I would be surprised if it did because you've not handled the issue in comment 1.
(Reporter)

Comment 8

5 years ago
(In reply to Jonathan Watt [:jwatt] from comment #6)
> It's cleaner not to have other classes reaching into private members IMO.

The frame classes do that throughout to get element attribute values.

> 
> > Every other type uses IsExplicitlySet so that's what viewBox should use
> > rather than HasRect or IsValid.
> 
> I disagree in this case, since IsExplicitlySet would sound like it should
> return true for viewBox="none", when actually what the callers are
> interested in is whether there is a rect defined.

viewBox="none" you're making that up! Or maybe just confusing viewBox with preserveAspectRatio.
(Reporter)

Comment 9

5 years ago
Comment on attachment 620680 [details] [diff] [review]
Only send the modification flags that are needed

It did indeed fail on try.
Attachment #620680 - Flags: review?(longsonr) → review-
(Assignee)

Comment 10

5 years ago
Created attachment 620704 [details] [diff] [review]
Clean up and document misleading viewBox code

I've gone with IsExplicitlySet as you want, but kept the nsSVGSVGElement method (shortened to just "HasViewBox" not that I've been corrected about the "none" value ;)) since I'd like to move away from reaching in to private members and it makes the code look cleaner.
Attachment #620704 - Flags: review?(longsonr)
(Assignee)

Updated

5 years ago
Attachment #620679 - Attachment is obsolete: true
(Assignee)

Comment 11

5 years ago
Err, forget the nsSVGSVGElement::ShouldSynthesizeViewBox XXX comment, I've removed that locally.
(Reporter)

Comment 12

5 years ago
Comment on attachment 620704 [details] [diff] [review]
Clean up and document misleading viewBox code

r=longsonr assuming the crazy comment is removed as stated in comment 11
Attachment #620704 - Flags: review?(longsonr) → review+
(Assignee)

Comment 13

5 years ago
Created attachment 620714 [details] [diff] [review]
Only send the change notification flags that are required
Attachment #620680 - Attachment is obsolete: true
Attachment #620714 - Flags: review?(longsonr)
(Reporter)

Updated

5 years ago
Attachment #620714 - Flags: review?(longsonr) → review+
(Assignee)

Comment 14

5 years ago
Pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b14c2b9b209
https://hg.mozilla.org/integration/mozilla-inbound/rev/683c20f1d5bd
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/8b14c2b9b209
https://hg.mozilla.org/mozilla-central/rev/683c20f1d5bd
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.