842181 - Text only zoom affects SVG text with svg.text.css-frames.enabled

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Robert Longson [:longsonr]]

Select text-only zoom mode and zoom in and out. Text size should not change.

Comment 1

[Cameron McCormack (:heycam)]

Rather than having to counteract the effects of text zoom in nsSVGTextFrame2, I wonder if it would be easier to teach the style structs etc. not to zoom SVG text font sizes.

Comment 2

[Robert Longson [:longsonr]]

If you can modify callers of nsStyleFont::ZoomText so that they skip that call for svg text that might work.

Note that for SVG text there's another wrinkle in that we need to ignore the UA's minimum font size too. That looks to be applied in nsRuleNode::SetFontSize so that will likely need some modification. So in http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleStruct.h#89 we eiether need to adjust anything that uses mFont.mSize and make it use mSize instead or alternatively skip the minimum font size logic that sets mFont.mSize so that for SVG text mSize == mFont.mSize

Comment 3

[Cameron McCormack (:heycam)]

dbaron, I was going to try having ZoomText() only be called for frames that are IsSVGText(), but it would result in me having to pass in two arguments (whether this frame is SVG text, and whether the parent frame is SVG text) into nsRuleNode::ComputeFontData(), since there are calls to ZoomText() under there.  nsRuleNode::ComputeFontData() needs to have the same arguments as all the other Compute*Data() methods.  And anyway I am getting the feeling that style computation needs to be independent of frames.

Do you have any suggestion on how to handle this?  Would it work somehow to record on a newly create nsStyleFont whether it should respond to zooms?  The reason we can't just handle this in nsSVGTextFrame2 (as we do currently with nsSVGGlyphFrame) is that all the ns{Block,Inline,Text}Frames need to be created at appropriate unzoomed text sizes so that they are laid out properly.

Comment 4

[Cameron McCormack (:heycam)]

roc, do you have any suggestions?

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It sounds like we need to have something in the style data that tracks whether the style context will be for SVG-text or not.

Comment 6

[Cameron McCormack (:heycam)]

Attachment: WIP (v0.1)

So this is what I have so far.  It partially works, in that mAllowZoom on the nsStyleFont for the anonymous block frame child (:-moz-svg-text) of the nsSVGTextFrame2 gets set to false, and that flag on ancestor frames is true.  What's not happening is the inheritance of this flag into the nsStyleFont of the child frame, be it an nsInlineFrame (corresponding to a <tspan>) or an nsTextFrame.  The "here #4" line doesn't actually get triggered, so I'm not even sure I'm getting inside WalkRuleTree for the child of the :-moz-svg-text frame.  What am I missing?

(The way I undo the zooming with nsStyleFont::EnableZoom() soon after the nsStyleFont is created should change to just not zooming inside nsStyleFont::Init in the first place.)

Comment 7

[Boris Zbarsky [:bzbarsky]]

If you're not getting into WalkRuleTree, where is the cached struct being found?  Is it already cached on a rulenode somewhere or something?

Comment 8

[Cameron McCormack (:heycam)]

I think... yes:

Breakpoint 1, nsSVGTextFrame2::PaintSVG (this=0x10d9eec48, aContext=0x15444acb0, aDirtyRect=0x0) at /z/moz/c/layout/svg/nsSVGTextFrame2.cpp:3273
3273	  printf("PaintSVG\n");
(gdb) p this->GetFirstPrincipalChild()->GetFirstPrincipalChild()
$6 = (nsInlineFrame *) 0x10d9ef248
(gdb) p $6->StyleContext()->mCachedInheritedData.mStyleStructs
$7 = {
  mArray = {0x10d9ef7a0, 0x0, 0x0, 0x10d9ef7f8, 0x10d9961a0, 0x0, 0x10d996088, 0x0, 0x10d9ef670}
}
(gdb) p $6->StyleFont()
$8 = (const nsStyleFont *) 0x10d9ef7a0
(gdb) p this->StyleFont()->mAllowZoom
$9 = true
(gdb) p this->GetFirstPrincipalChild()->StyleFont()->mAllowZoom
$10 = false
(gdb) p $6->StyleFont()->mAllowZoom
$11 = true

Comment 9

[Cameron McCormack (:heycam)]

Or am I misunderstanding, and that's cached style contexts from a parent.  The rule node doesn't have any cached inherited style data:

(gdb) p this->GetFirstPrincipalChild()->GetFirstPrincipalChild()->StyleContext()->mRuleNode->mStyleData
$3 = {
  mInheritedData = 0x0,
  mResetData = 0x10dd87738
}

Comment 10

[Boris Zbarsky [:bzbarsky]]

The cached stuff on a style context could have come from the parent or from the rule.

But normally you can't get stuff off your parent without calling WalkRuleTree first, since that's what checks for rules that might affect you.

Looking at that printf again, why would something have an anon box style as its parent?  That should never happen except maybe for other anon boxes.

Comment 11

[Cameron McCormack (:heycam)]

(In reply to Boris Zbarsky (:bz) from comment #10)
> But normally you can't get stuff off your parent without calling
> WalkRuleTree first, since that's what checks for rules that might affect you.
> 
> Looking at that printf again, why would something have an anon box style as
> its parent?  That should never happen except maybe for other anon boxes.

Oh, I see.  So my nsInlineFrame's style context's parent is the nsSVGTextFrame2's style context, not the anonymous box.  OK.

So in WalkRuleTree I'm using the ->GetPseudo() to determine how to set mAllowZoom.  I can't check the actual frame type, since we don't have frames anywhere around here.  What would be the best way to pass some state in here so that I can instead set mAllowZoom to true for the nsSVGTextFrame2's style context, and let it inherit from there?  A new style bit that I can set on any newly created style context for an nsSVGTextFrame2?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Hmm.  A new bit on style contexts seems like the right way to go, yeah...

Comment 13

[Cameron McCormack (:heycam)]

Attachment: WIP (v0.2)

This is half way there.  It works on all of the text elements in http://www.w3.org/Graphics/SVG/Test/20110816/svg/text-text-03-b.svg but not the "$Revision: 1.1 $" for some reason.

Comment 14

[Cameron McCormack (:heycam)]

And I think the key to that one element that's still responding to the text zoom is that it doesn't have any other font properties set on it.

Comment 15

[Cameron McCormack (:heycam)]

Attachment: patch (v1.0)

All right, this seems to be working.

I was out of flag bits in nsStyleContext::mBits, so I decided to unpack the flags and the pseudo type from the inherited style structs bitfield.

Am I right in not needing to set canStoreInRuleTree?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 738450 [details] [diff] [review]
patch (v1.0)

I actually don't know offhand.  Would like dbaron to review this....

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 738450 [details] [diff] [review]
patch (v1.0)

(Please read to the end of the review first; the first few comments end up becoming irrelevant later on.)

It looks like the mDisallowTextZoom on the TreeMatchContext is set only
in nsFrameManager::ReResolveStyleContext, which handles dynamic style
reresolution, and not in the initial style resolution codepath.  This
seems wrong, and likely to produce incorrect results if the user loads a
page with text zoom already set.

>-  if (!ruleData.mCanStoreInRuleTree)
>+  nsStyleContext* parentContext = aContext->GetParent();
>+
>+  if (!ruleData.mCanStoreInRuleTree ||
>+      (aSID == eStyleStruct_Font &&
>+       parentContext &&
>+       parentContext->IsTextZoomDisallowed() !=
>+         aContext->IsTextZoomDisallowed())) {
>     detail = eRulePartialMixed; // Treat as though some data is specified to avoid
>                                 // the optimizations and force data computation.
>+  }

Instead of going here, this logic should live in CheckFontCallback,
which is called by CheckSpecifiedProperties, just a few lines above
where you added this.

I think it should be sufficient to map:
  None -> PartialMixed
  PartialInherited -> PartialMixed
  PartialReset -> PartialMixed
  FullInherited -> FullMixed
  FullReset -> FullMixed
since you need to ensure that we don't assume we can copy the parent's
data unchanged (i.e., we can't use None, PartialReset, and ...


Except I don't think this logic works.  You're only doing part of the
"can't store in rule tree" logic -- you're not doing anything to block
existing cached style data in the rule tree from applying to SVG text;
you're only preventing caching the result from SVG text and applying
it elsewhere, and even then only if the parent isn't SVG text.

Slightly more formally (maybe, if my attempt at formality works):  if in
a particular condition V (a set of values coming from the style rules),
a condition C that is independent of V might be different between
different style contexts pointing to the same rule node and cause
different style data to result (in other words, that C∧V and (¬C)∧V
yield different style data), then you need to set canStoreInRuleTree to
false whenever V is true.  It can't be conditioned on C; if it is, we'll
store the data in the rule tree for the ¬C case and then use it for the
C case.  Here V is the set of any value of font-size (¬V is empty).  But
you're only setting canStoreInRuleTree to false (implicitly, by changing
detail) when C is true.



I'd suggest an alternative approach to messing with any of the stuff
I've commented on above, i.e., both the mDisallowTextZoom and the
changes to WalkRuleTree (and also the new bits on the style context and
all the logic related to it, and the changes to nsRuleNode::SetFont):
add a new "internal-only" CSS property (much like the existing -x-lang)
that is set on svg:text by some C++ code in nsStyleSet (much like the
pseudo-restriction rule code there, except in ResolveStyleFor instead of
{Resolve,Probe}PseudoStyleFor, and probably at the lowest level of the
cascade rather than the highest).  The rule implementation would set
this one property to eCSSUnit_None.  Then ComputeFontData would compute
this property just like any other (before calling SetFont or
SetGenericFont), and SetFontSize (which is called from SetFont or
SetGenericFont) would use it.

(Also, on an unrelated note that I stumbled across while looking at code
to review this:  when SVG text is enabled, will ::-moz-svg-text go away
and be replaced by a rule that matches svg:text?  It's not clear to me
why that's an anon box rather than an element, other than perhaps to
deal with the temporary condition of not knowing whether to apply it.)


Sorry for taking so long to get to this.  (I felt like there was a lot
to check here; that feeling probably should have led me to more quickly
suggest that it was the wrong approach.)

Comment 18

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (in W3C meetings through June 11) (don't cc:, use needinfo? instead) from comment #17)
> I'd suggest an alternative approach to messing with any of the stuff
> I've commented on above, i.e., both the mDisallowTextZoom and the
> changes to WalkRuleTree (and also the new bits on the style context and
> all the logic related to it, and the changes to nsRuleNode::SetFont):
> add a new "internal-only" CSS property (much like the existing -x-lang)
> that is set on svg:text by some C++ code in nsStyleSet (much like the
> pseudo-restriction rule code there, except in ResolveStyleFor instead of
> {Resolve,Probe}PseudoStyleFor, and probably at the lowest level of the
> cascade rather than the highest).  The rule implementation would set
> this one property to eCSSUnit_None.  Then ComputeFontData would compute
> this property just like any other (before calling SetFont or
> SetGenericFont), and SetFontSize (which is called from SetFont or
> SetGenericFont) would use it.

That sounds fair enough.  We still need to keep the mDisallowTextZoom on the style struct as the place to store the result of computing the internal CSS property, though, right?

> (Also, on an unrelated note that I stumbled across while looking at code
> to review this:  when SVG text is enabled, will ::-moz-svg-text go away
> and be replaced by a rule that matches svg:text?  It's not clear to me
> why that's an anon box rather than an element, other than perhaps to
> deal with the temporary condition of not knowing whether to apply it.)

No, this was intended to stay around.  It targets the anonymous child box of the nsSVGTextFrame2 -- the block frame in which the child text nodes and inline frames (corresponding to <tspan> elements) are laid out.

Comment 19

[Cameron McCormack (:heycam)]

Attachment: patch v2

I wasn't really sure how to walk my text zoom property setting rule at a low cascade level rather than a high one like the restriction rules do.  Which level constant would I use?  This patch just uses eAgentSheet, and walks it before walking all the normal rules.

Since we're just looking at whether the element is <svg:text>, rather than whether the frame IsSVGText(), this affects the old SVG text frame code too.  So the patch also removes the dividing by the text zoom factor in nsSVGGlyphFrame.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=9c298e447225

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 760330 [details] [diff] [review]
patch v2

>I wasn't really sure how to walk my text zoom property setting rule at a
>low cascade level rather than a high one like the restriction rules do.
>Which level constant

That's fine; what you did is a low cascade level.

(The restriction rules don't actually create the restrictions; they just
force things with the restrictions to be in a different branch of the
rule tree.)


You should at least put an assertion in nsStyleFont::CalcDifference that
mAllowZoom is the same on both.

CanvasRenderingContext2D.cpp:

>-  const nscoord fontSize = nsStyleFont::UnZoomText(parentContext->PresContext(), fontStyle->mSize);
>+  const nscoord fontSize = fontStyle->mAllowZoom ?
>+    nsStyleFont::UnZoomText(parentContext->PresContext(), fontStyle->mSize) :
>+    fontStyle->mSize;

Even better, you can get rid of UnZoomText entirely if you append the
style set's DisableTextZoomStyleRule rule to |rules| higher up.

nsRuleNode.cpp:

>-  nscoord minScriptSize =
>-    nsStyleFont::ZoomText(aPresContext, aParentFont->mScriptMinSize);
>+  nscoord minScriptSize = aParentFont->mScriptMinSize;
>+  if (aFont->mAllowZoom) {
>+    minScriptSize = nsStyleFont::ZoomText(aPresContext,
>+                                          aParentFont->mScriptMinSize);
>+  }

Inside the aFont->mAllowZoom block, I think it's clearer to use
minScriptSize as both input and output (rather than getting
aParentFont->mScriptMinSize again).

In ComputeFontData, I'd prefer making it more like a normal CSS
property.  In other words, handle initial, inherit, and none explicitly
to set font->mAllowZoom to true, the inherited value, and false.  Remove
nsStyleFont::EnableZoom(), and do any adjustments needed directly in
this code.  (I think the current code probably misbehaves in some of the
start-struct cases, which are tricky.)

(I think I'd like to look at this bit again, unfortunately.)

nsStyleSet.cpp:

>+/* virtual */ void
>+nsTextZoomStyleRule::MapRuleInfoInto(nsRuleData* aRuleData)
>+{
>+  if (!(aRuleData->mSIDs & NS_STYLE_INHERIT_BIT(Font)))
>+    return;
>+  aRuleData->ValueForTextZoom()->SetNoneValue();
>+}

Please make this more like the normal case as well:  don't touch the
value unless it's currently eCSSUnit_Null.



Finally, could you search replace TextZoomStyleRule -> DisableTextZoomStyleRule?


r=dbaron, though I think I want to look at ComputeFontData again.  (When you make a new review request, please remind me of that in the request so I remember it's just a rereview of a small part.)

Comment 21

[Cameron McCormack (:heycam)]

Attachment: patch v2.1

Addressed all the comments.  You just wanted to review the changes I made to ComputeFontData.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 764498 [details] [diff] [review]
patch v2.1

>+  // -x-text-zoom: none, inherit, initial
>+  bool allowZoom;
>+  const nsCSSValue* textZoomValue = aRuleData->ValueForTextZoom();
>+  if (eCSSUnit_Inherit == textZoomValue->GetUnit()) {
>+    allowZoom = parentFont->mAllowZoom;
>+  } else if (eCSSUnit_None == textZoomValue->GetUnit()) {
>+    allowZoom = false;
>+  } else {
>+    MOZ_ASSERT(eCSSUnit_Initial == textZoomValue->GetUnit() ||
>+               eCSSUnit_Null == textZoomValue->GetUnit(),
>+               "unexpected unit");
>+    allowZoom = true;
>+  }
>+  font->EnableZoom(mPresContext, allowZoom);

So setting allowZoom to true in the eCSSUnit_Null case is definitely wrong; the patch will end up producing different results depending on which styles have been computed previously.  Basically, you need to ensure that if you have eCSSUnit_Null, you don't touch anything.  That's one of the rules of the nsRuleNode::Compute*Data methods.  So you need an else if rather than just an else, and you should only call EnableZoom if you hit one of those 3 cases and not the eCSSUnit_Null case.

I suppose that seems safe, though I'm still vaguely suspicious of EnableZoom itself.

Comment 23

[Cameron McCormack (:heycam)]

Attachment: patch v2.3

I discovered that the effect of the call to nsStyleFont::EnableZoom(false) was being overwritten by nsRuleNode::SetGenericFont.  So I've moved the -x-text-zoom computation out of nsRuleNode::ComputeFontData to the top of nsRuleNode::SetFont.

Try run in progress: https://tbpl.mozilla.org/?tree=Try&rev=20b3f28e206c

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Cameron McCormack (:heycam) from comment #23)
> I discovered that the effect of the call to nsStyleFont::EnableZoom(false)
> was being overwritten by nsRuleNode::SetGenericFont.  So I've moved the
> -x-text-zoom computation out of nsRuleNode::ComputeFontData to the top of
> nsRuleNode::SetFont.

That sounds a bit fishy.  In general, you can't depend on the ordering, since you might be running ComputeFontData with a partial (higher in the cascade) set of rules, starting from a struct copy-constructed from style data computed from lower (in the cascade) rules.  (Though since the -x-text-zoom is always lowest, you can *sort of* depend on the ordering, but I'd still prefer you not do so.)

Comment 25

[Cameron McCormack (:heycam)]

Maybe I should add an nsDisableTextZoomStyleRule to the parent context stuff here, then?

  https://mxr.mozilla.org/mozilla-central/source/content/canvas/src/CanvasRenderingContext2D.cpp#2062

That is where it's copying the nsStyleFont data across from.

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 764560 [details] [diff] [review]
patch v2.3


>+/* virtual */ void
>+nsDisableTextZoomStyleRule::MapRuleInfoInto(nsRuleData* aRuleData)
>+{
>+  if (!(aRuleData->mSIDs & NS_STYLE_INHERIT_BIT(Font)))
>+    return;
>+
>+  nsCSSValue* value = aRuleData->ValueForTextZoom();
>+  if (value->GetUnit() != eCSSUnit_Null)
>+    return;
>+
>+  aRuleData->ValueForTextZoom()->SetNoneValue();
>+}

Also, could you invert the != and indent the SetNoneValue() call.  We have dozens of lines in the tree using the pattern:
  if (value->GetUnit() == eCSSUnit_Null)
    value->Set...();
and I'd rather stick to that pattern unless there's a reason not to.  (Note that that also reuses |value|, which you should also do.)

Comment 27

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #26)
> Also, could you invert the != and indent the SetNoneValue() call.  We have
> dozens of lines in the tree using the pattern:
>   if (value->GetUnit() == eCSSUnit_Null)
>     value->Set...();

My pleasure, since that's my preferred way of writing it.  When I was searching for eCSSUnit_Inherit or something in the file, I came across that reversed style, and assumed it must be the local style (without checking the rest of the file).

> and I'd rather stick to that pattern unless there's a reason not to.  (Note
> that that also reuses |value|, which you should also do.)

OK.

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Cameron McCormack (:heycam) from comment #25)
> Maybe I should add an nsDisableTextZoomStyleRule to the parent context stuff
> here, then?
> 
>  
> https://mxr.mozilla.org/mozilla-central/source/content/canvas/src/
> CanvasRenderingContext2D.cpp#2062
> 
> That is where it's copying the nsStyleFont data across from.

You shouldn't need to.  I think as long as:
 1. EnableZoom swaps the zoomedness of the values, as it currently does
 2. everything that sets one of the values follows mEnableZoom
things should work.

I suspect the problem may be that you're missing cases for (2), in particular, there are cases where we don't do any zooming right now because the value we're copying is already zoomed, which essentially assumes that this->mEnableZoom is the same as parent->mEnableZoom.  Since that assumption is false, I suspect nsRuleNode::SetFontSize needs some more adjustment.

It might be that we can prove that some cases aren't needed, but I'd rather just do things under the normal invariants.

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And you might have to be careful of what COMPUTE_START_INHERITED does when setting up parentContext / parentdata.  It's possible some of those quirks might matter, though I hope they don't.

Comment 30

[Cameron McCormack (:heycam)]

Ultimately what is happening is:

  * we are using a generic font for the canvas
  * ComputeFontData will initially set mAllowZoom = false correctly on the nsStyleFont
  * when we then call SetGenericFont, this copies over all the nsStyleFont data from
    an ancestor nsStyleContext, which includes mAllowZoom = true
  * we then trigger the assertion I've added in CanvasRenderingContext2D::SetFont

I'm feeling now like it makes sense to call EnableZoom again towards the bottom of nsRuleNode::SetGenericFont, since we'll have copied some zoomed font information from an ancestor, and we need to unzoom it (and thus set mAllowZoom = false) on the nsStyleFont we're computing.  Does that sound right?

Comment 31

[Cameron McCormack (:heycam)]

Attachment: patch v2.4

Like this.

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Cameron McCormack (:heycam) from comment #30)
> I'm feeling now like it makes sense to call EnableZoom again towards the
> bottom of nsRuleNode::SetGenericFont, since we'll have copied some zoomed
> font information from an ancestor, and we need to unzoom it (and thus set
> mAllowZoom = false) on the nsStyleFont we're computing.  Does that sound
> right?

This doesn't seem right (nor does adding the disable rule the extra time in the canvas code).  The problem is that you *might* have copied some zoomed font information from an ancestor, or you might not.

I think, fundamentally, the problem is that the semantics of the |zoom| variable in nsRuleNode::SetFontSize need to change.  Right now zoom==false means that the value is already zoomed, and zoom==true means it needs to be zoomed at the end.

The problem is that we instead need a variable that says "this is zoomed based on the parent's mAllowZoom".  Then, at the end, we should consider (a) the parent's mAllowZoom (b) this mAllowZoom and (c) this variable, to determine whether to zoom, unzoom, or do nothing.  SetFontSizeCalcOps::ComputeLeafValue also would need adjustment to match those semantics, to only call ZoomText when the parent's mAllowZoom is true.  (I think it's simplest to pass only the parent's mAllowZoom to SetFontSizeCalcOps, and not pass this's mAllowZoom to it.)

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 764568 [details] [diff] [review]
patch v2.4

see comment 32.  Sorry for not getting to this yesterday.

Comment 34

[Cameron McCormack (:heycam)]

Attachment: patch v3

Another shot, with the changes to the zoom local variable in nsRuleNode::SetFontSize.

I think again that we need to handle -x-text-zoom in nsRuleNode::SetFont rather than ComputeFontData, since we need to also set mAllowZoom when we call SetGenericFont just before it would call SetFontSize.

Comment 35

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 768966 [details] [diff] [review]
patch v3

>+  //
>+  // parent zoomed?  aFont wants zoomed?  sizeIsZoomedAccordingToParent  action
>+  // ---------------------------------------------------------------------------
>+  //     false             false                    false                nothing
>+  //     false             false                    true                 nothing
>+  //     false             true                     false                zoom
>+  //     false             true                     true                 zoom
>+  //     true              false                    false                nothing
>+  //     true              false                    true                 unzoom
>+  //     true              true                     false                zoom
>+  //     true              true                     true                 nothing
>+  if ((aFont->mAllowZoom && !aParentFont->mAllowZoom) ||
>+      (aFont->mAllowZoom && aParentFont->mAllowZoom &&
>+       !sizeIsZoomedAccordingToParent)) {
>     *aSize = nsStyleFont::ZoomText(aPresContext, *aSize);
>+  } else if (!aFont->mAllowZoom && aParentFont->mAllowZoom &&
>+             sizeIsZoomedAccordingToParent) {
>+    *aSize = nsStyleFont::UnZoomText(aPresContext, *aSize);
>   }


I don't think it should be this complicated.  In particular, I think the following is simpler, clearer, and doesn't even require comments (and works out to exactly the same thing):

  bool currentlyZoomed = sizeIsZoomedAccordingToParent && aParentFont->mAllowZoom;
  if (!currentlyZoomed && aFont->mAllowZoom) {
    *aSize = nsStyleFont::ZoomText(aPresContext, *aSize);
  } else if (currentlyZoomed && !aFont->mAllowZoom) {
    *aSize = nsStyleFont::UnZoomText(aPresContext, *aSize);
  }


>@@ -3022,16 +3047,31 @@ static int8_t ClampTo8Bit(int32_t aValue) {
> nsRuleNode::SetFont(nsPresContext* aPresContext, nsStyleContext* aContext,
>                     uint8_t aGenericFontID, const nsRuleData* aRuleData,
>                     const nsStyleFont* aParentFont,
>                     nsStyleFont* aFont, bool aUsedStartStruct,
>                     bool& aCanStoreInRuleTree)
> {
>   bool atRoot = !aContext->GetParent();
> 
>+  // -x-text-zoom: none, inherit, initial

Yeah, I think you're right that this needs to be in SetFont.

>+/* virtual */ void
>+nsDisableTextZoomStyleRule::MapRuleInfoInto(nsRuleData* aRuleData)
>+{
>+  if (!(aRuleData->mSIDs & NS_STYLE_INHERIT_BIT(Font)))
>+    return;
>+
>+  nsCSSValue* value = aRuleData->ValueForTextZoom();
>+  if (value->GetUnit() != eCSSUnit_Null)
>+    return;
>+
>+  aRuleData->ValueForTextZoom()->SetNoneValue();

Could you flip these last 4 lines as I said in comment 26?


>+#ifdef DEBUG
>+/* virtual */ void
>+nsDisableTextZoomStyleRule::List(FILE* out, int32_t aIndent) const
>+{
>+  for (int32_t index = aIndent; --index >= 0; ) fputs("  ", out);
>+  fputs("[text zoom style rule] {}\n", out);

"disable text zoom"


r=dbaron with that

Comment 36

[Cameron McCormack (:heycam)]

Thanks David!

https://hg.mozilla.org/integration/mozilla-inbound/rev/0c7b39985a22

Comment 37

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0c7b39985a22