Closed Bug 1055667 Opened 5 years ago Closed 5 years ago

test and implement correct behavior of margin/border/padding on ruby elements

Categories

(Core :: Layout: Block and Inline, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: dbaron, Assigned: xidorn)

References

Details

Attachments

(9 files, 6 obsolete files)

1.10 KB, text/html
Details
63.72 KB, image/png
Details
4.51 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
5.84 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
7.23 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
2.60 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
6.31 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
4.28 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
8.83 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
We should add tests for the correct behavior of margin, border, and padding on ruby elements.  It's not completely clear what they should do, particularly in the vertical direction (where they could be more like inlines or more like inline-blocks, although I'd prefer more like inlines).  This may depend on the spec specifying correct behavior.

The current code may well be in pretty good shape, though.
Here's a testcase with a border on various <ruby> elements.

Right now, a sufficiently-large border on any ruby element will overlap either that element's own contents, or the contents of an adjacent element. Some of this probably needs to be addressed before enabling ruby -- particularly in cases where the border overlaps the same element's own content.

I tried a similar local example with <span> elements (and no ruby), and there, it looks like normal inline-level elements' borders can cause similar overlap in the block direction (so, the border may overlap content in the previous/next line), but not in the inline direction (so, a <span>'s border won't overlap content to the right of it on the same line).
Here's a screenshot of testcase 1, with the particularly-needing-to-be-fixed cases of overlap highlighted w/ arrows.  (where "particularly-needing-to-be-fixed" is defined as "inline-direction", based on comparison to <span> behavior as discussed in previous comment.)

Note that in the "border on rtc" example, the "xx" and "yy" characters themselves are overlapping a bit. (i.e. one <rt> is overlapping the next <rt>, squeezed by their <rtc>.)
Depends on: 1055658
No longer depends on: 1055658
My plan of implementing this bug is:

1. Allows margin/border/padding on rb/rt boxes.
They are simply subclass of nsInlineFrame, hence it is almost done.

2. Suppress border/padding on rbc/rtc boxes.
It is not trivial to implement border/padding on these boxes.
These boxes behave like inline frames, especially given that they can be broken inside. However, we still have bug 122795 open, which means our border/padding support for inline frames is not that complete yet. I don't think it worth adding another immature impl here.
In addition, we can't even keep the current immature impl, because whatever padding/border added, in my opinion, it is still necessary to keep content of the boxes aligned among levels. This will require a bit more code than just an simple immature impl.

3. Allows margin on rbc/rtc boxes.
The inline direction margin has been handled by nsLineLayout, and the block direction could be handled in nsRubyFrame manually. I'm not sure should I do this. It seems that block direction margin has no effect on inline elements?

4. Allow margin on ruby boxes.
It has been handled by nsLineLayout. I don't think any additional thing needs to be done for it.

5. Leave the current immature impl of border/padding of ruby boxes.
Same reason with rbc/rtc boxes. But the current immature impl for ruby boxes doesn't that broken when we don't break inside. And when there is a line break, it is just a little worse than the nsInlineFrame (padding-start is always applied here). So I think it's fine to leave it as it is now.

Do you agree with this plan? Or do you have any concerns about it?
Flags: needinfo?(dbaron)
How does that match what the spec currently says?
Flags: needinfo?(dbaron)
The spec discusses this problem in this section: http://dev.w3.org/csswg/css-ruby/#box-style

The spec does neither require nor forbid UAs to support any of border/padding/margin/background/outline on rbc and rtc. And other boxes should just behavior like a normal inline frame.

I think my plan completely matches the spec, except that <ruby> has a more flawed impl of border/padding than inline frame (which, I think, is easy to be fixed to as flawed as inline frame, but I wonder whether it is worth to fix now. I'd prefer we fix it when we get a mature impl for nsInlineFrame)

In addition, if the plan is to suppress border/padding on rbc/rtc, may I also suppress background/outline via overridding BuildDisplayList? Currently, the code for building display list for background and border seems to be bound together. I see no reason we should separate them just for rbc/rtc on which we are not required to implement these properties.
Flags: needinfo?(dbaron)
(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #3)
> My plan of implementing this bug is:
> 
> 1. Allows margin/border/padding on rb/rt boxes.
> They are simply subclass of nsInlineFrame, hence it is almost done.

OK.

> 2. Suppress border/padding on rbc/rtc boxes.
> It is not trivial to implement border/padding on these boxes.
> These boxes behave like inline frames, especially given that they can be
> broken inside. However, we still have bug 122795 open, which means our
> border/padding support for inline frames is not that complete yet. I don't
> think it worth adding another immature impl here.
> In addition, we can't even keep the current immature impl, because whatever
> padding/border added, in my opinion, it is still necessary to keep content
> of the boxes aligned among levels. This will require a bit more code than
> just an simple immature impl.

I don't think the existence of bug 122795 justifies this; it's quite obscure.  However, I'm OK with this anyway.

> 3. Allows margin on rbc/rtc boxes.
> The inline direction margin has been handled by nsLineLayout, and the block
> direction could be handled in nsRubyFrame manually. I'm not sure should I do
> this. It seems that block direction margin has no effect on inline elements?

That's correct; block-direction margin has no effect on inline elements.  So it sounds like there's nothing to do here.

> 4. Allow margin on ruby boxes.
> It has been handled by nsLineLayout. I don't think any additional thing
> needs to be done for it.

OK.

> 5. Leave the current immature impl of border/padding of ruby boxes.
> Same reason with rbc/rtc boxes. But the current immature impl for ruby boxes
> doesn't that broken when we don't break inside. And when there is a line
> break, it is just a little worse than the nsInlineFrame (padding-start is
> always applied here). So I think it's fine to leave it as it is now.

You should at least file a bug on fixing the border/padding always being applied after the break.  It doesn't need to block enabling ruby, but we should have a bug on file.



In addition to the above, you should have some tests testing the behavior.
Flags: needinfo?(dbaron)
... and the tests could perhaps test backgrounds at the same time.
So do you agree to suppress background and outline on rbc/rtc as well?
Flags: needinfo?(dbaron)
I suppose I'd be OK with it; I'm not happy about it, though.  However, I think I might prefer supporting background and outline even if we're suppressing margin, border, and padding.
Flags: needinfo?(dbaron)
So does it sound acceptable to have border painted without leaving space for it?
Flags: needinfo?(dbaron)
No.
Flags: needinfo?(dbaron)
The outline of ruby text boxes depends on the overflow area fix in bug 1055658.
Depends on: 1055658
Assignee: nobody → quanxunzhen
Attachment #8547354 - Flags: review?(dbaron)
Attachment #8547361 - Flags: review?(dbaron)
Attachment #8547363 - Flags: review?(dbaron)
Attachment #8547363 - Attachment description: patch 7 - Fix border/padding of ruby frame → patch 7 - Reftests for box properties on ruby boxes
Attachment #8547369 - Flags: review?(dbaron)
Attachment #8547354 - Flags: review?(dbaron) → review+
Comment on attachment 8547355 [details] [diff] [review]
patch 2 - Remove unnecessary param of SetBSizeFromFontMetrics

Either you have a patch elsewhere in your queue removing the need for it, or you also need to change nsRubyBaseContainerFrame.

r=dbaron either way
Attachment #8547355 - Flags: review?(dbaron) → review+
Comment on attachment 8547360 [details] [diff] [review]
patch 3 - Suppress border/padding space on rbc/rtc frames

I guess this is ok, but in addition to this (or maybe instead of it) you want to set border and padding to 0 in  nsCSSOffsetState::InitOffsets and then override GetUsedBorder and GetUsedPadding like nsTableFrame does.

The removal of the aReflowState parameter to nsLayoutUtils::SetBSizeFromFontMetrics belongs in the previous patch.

r=dbaron with that; I should probably review the additional changes as a separate patch
Attachment #8547360 - Flags: review?(dbaron) → review+
Comment on attachment 8547361 [details] [diff] [review]
patch 4 - Add base class for rbc & rtc

Maybe comment next to the protected constructor that it is not expected to be instantiated on its own; only its derived classes are expected to be instantiated.

r=dbaron with that
Attachment #8547361 - Flags: review?(dbaron) → review+
Comment on attachment 8547362 [details] [diff] [review]
patch 5 - Stop building display items for borders on ruby level containers

This seems like a future maintainability problem; it would be better not to create an extra copy of this code.

I wonder if the code that creates the border display item could detect that nothing should be done because of a GetUsedBorder override that returns 0 width?  (Might it already?)

It also breaks visibility:visible descendants of a visibility:hidden rtc/rbc, since the IsVisibleForPainting check is wrong.
Attachment #8547362 - Flags: review?(dbaron) → review-
(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #24)
> Comment on attachment 8547362 [details] [diff] [review]
> patch 5 - Stop building display items for borders on ruby level containers
> 
> This seems like a future maintainability problem; it would be better not to
> create an extra copy of this code.

So what should I do for this.

> I wonder if the code that creates the border display item could detect that
> nothing should be done because of a GetUsedBorder override that returns 0
> width?  (Might it already?)

It seems the answer is no, at least not already. I move the overriding code from patch 8, but the border is still painted. Any suggestion?
Flags: needinfo?(dbaron)
Hmmm.  Is it really going to be simpler to suppress the border than to just implement border and padding correctly?
Flags: needinfo?(dbaron)
(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #26)
> Hmmm.  Is it really going to be simpler to suppress the border than to just
> implement border and padding correctly?

It is unclear what is the correct behavior for rbc and rtc, especially considering that they need to be aligned among levels and they also have aligned children.

There are several options, roughly either align the content boundary and move rtc or rbc accordingly, or align border boundary and extend the padding in some levels. The first opinion looks more correct, but requires more work to align, and could potentially make it harder to check line breaking. The second option at least requires one more loop to get the maximum border/padding.

The other reason I have mentioned is that it is unclear to me what will be done to make normal inline frames have proper border/padding behavior. Since the border/padding in ruby level containers would be more complicated than other inline frames, if we leave an immature impl here, we might need to do much more work when we want to fix it someday in the future.

Given the reasons, I still prefer to suppress instead of to implement them.

I didn't want to create an extra copy of that code, that's the reason I asked whether I can suppress background and outline as well.

There is another opinion to do the suppression, which is to compute border/padding to zero in nsRuleNode or nsStyleCotext::ApplyStyleFixups. That will cancel any potential effect of border/padding, and we can keep the related code together. Given that padding and border have their own style struct, I guess it would not be a big waste to fix them up in ApplyStyleFixups. Sounds good?
Flags: needinfo?(dbaron)
s/opinion/option/ in the last paragraph...
What about this? I guess it makes code more compact as we don't need to put the suppression everywhere to eliminate the effect.

I'm considering putting shared empty nsStyleBorder and nsStylePadding in nsPresContext, so that we don't need to create a new struct for every rbc and rtc.
Attachment #8548516 - Flags: feedback?(dbaron)
The new patch can take place of the current patch 4 and patch 5.
Comment on attachment 8547369 [details] [diff] [review]
patch 6 - Fix border/padding of ruby frame

This should only add the start-padding on if it's the first continuation or box-decoration-break is clone, and the end-padding on if it's reporting complete or box-decoration-break is clone.  (See nsInlineFrame::ReflowFrames, although you don't need to worry about the block-in-inline split cases, and I'm not sure why it tests !LastInFlow()->GetNextContinuation().)

You should also add reftests for both (a) what this is fixing and (b) some of the splitting cases.
Attachment #8547369 - Flags: review?(dbaron) → review-
Comment on attachment 8547354 [details] [diff] [review]
patch 1 - Allow inline direction margin of ruby base/text boxes

You should also add a reftest for this.
(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #32)
> Comment on attachment 8547354 [details] [diff] [review]
> patch 1 - Allow inline direction margin of ruby base/text boxes
> 
> You should also add a reftest for this.

I have had reftests for all those things in patch 7.
Comment on attachment 8547363 [details] [diff] [review]
patch 7 - Reftests for box properties on ruby boxes

r=dbaron, although I didn't look very closely.

(And, actually, maybe most of the reftests I requested in the previous two comments are covered here,except for the ones for the additional changes I asked for for patch 6.  Are they?)
Attachment #8547363 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #34)
> Comment on attachment 8547363 [details] [diff] [review]
> patch 7 - Reftests for box properties on ruby boxes
> 
> r=dbaron, although I didn't look very closely.
> 
> (And, actually, maybe most of the reftests I requested in the previous two
> comments are covered here,except for the ones for the additional changes I
> asked for for patch 6.  Are they?)

No, they aren't. I'll add.
(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #29)
> What about this? I guess it makes code more compact as we don't need to put
> the suppression everywhere to eliminate the effect.

I'll think about this.

> I'm considering putting shared empty nsStyleBorder and nsStylePadding in
> nsPresContext, so that we don't need to create a new struct for every rbc
> and rtc.

I don't think that's worth the code complexity; style struct ownership is complicated enough already.



One issue none of these suppression patches really handle is what to do if the theme (-moz-appearance) forces border or padding.  I guess we just don't worry about it, but it's not ideal.
(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #30)
> The new patch can take place of the current patch 4 and patch 5.

Also patch 8?
(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #37)
> (In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #30)
> > The new patch can take place of the current patch 4 and patch 5.
> 
> Also patch 8?

Yes, I forgot that.
Comment on attachment 8548516 [details] [diff] [review]
patch to compute border/padding to zero for rbc/rtc

OK, let's do this instead of patches 4, 5, and 8.

It still doesn't handle the native theme issue, but I guess that's ok.  (The native theme issue might be a little better if we skipped patch 3, perhaps?)
Flags: needinfo?(dbaron)
Attachment #8548516 - Flags: review+
Attachment #8548516 - Flags: feedback?(dbaron)
Attachment #8548516 - Flags: feedback+
Attachment #8547371 - Flags: review?(dbaron)
(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #39)
> Comment on attachment 8548516 [details] [diff] [review]
> patch to compute border/padding to zero for rbc/rtc
> 
> OK, let's do this instead of patches 4, 5, and 8.

I'm a bit concerned about the memory usage. I believe this impl always create a new struct even if we currently inherit an already empty one. Or maybe we always have an independent empty struct for reset style if it is not explicitly marked inherit?

> It still doesn't handle the native theme issue, but I guess that's ok.  (The
> native theme issue might be a little better if we skipped patch 3, perhaps?)

I need to look into what the theme would do for this.

I don't think thing would be better if we skip patch 3. I think it would just make the aligning odd, instead of simply have border without space for them.

Maybe we can also fixup -moz-appearance here?
Flags: needinfo?(dbaron)
(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #40)
> (In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from
> comment #39)
> > Comment on attachment 8548516 [details] [diff] [review]
> > patch to compute border/padding to zero for rbc/rtc
> > 
> > OK, let's do this instead of patches 4, 5, and 8.
> 
> I'm a bit concerned about the memory usage. I believe this impl always
> create a new struct even if we currently inherit an already empty one. Or
> maybe we always have an independent empty struct for reset style if it is
> not explicitly marked inherit?

You could restructure the patch to only call GetUniqueStyleData if the padding/border is nonzero.  Don't try any new ownership schemes, please.

> Maybe we can also fixup -moz-appearance here?

No; don't bother.
Flags: needinfo?(dbaron)
Attachment #8547361 - Attachment is obsolete: true
Attachment #8547362 - Attachment is obsolete: true
Attachment #8547371 - Attachment is obsolete: true
Attachment #8548516 - Attachment is obsolete: true
Attachment #8548577 - Flags: review?(dbaron)
Simplifies some code stolen from nsInlineFrame. Also makes the frame act more like an inline frame in other methods we didn't copy.
(I wanted to copy GetLogicalSkipSides, but then I guess it would be better to make it inherit.)
Attachment #8548639 - Flags: review?(dbaron)
Attachment #8547369 - Attachment is obsolete: true
Attachment #8548640 - Flags: review?(dbaron)
Add one test for border/padding of ruby frames when there are line breaks.
Attachment #8547363 - Attachment is obsolete: true
Attachment #8548641 - Flags: review?(dbaron)
Comment on attachment 8548577 [details] [diff] [review]
patch 4 - Compute border/padding to zero for rbc/rtc

>+    if (StyleBorder()->GetComputedBorder() != nsMargin()) {

>+        computedPadding != nsMargin()) {

I'd prefer that these be written as nsMargin(0, 0, 0, 0) rather than as nsMargin(), since it's clearer what the value being compared to is.

r=dbaron with that
Attachment #8548577 - Flags: review?(dbaron) → review+
Comment on attachment 8548639 [details] [diff] [review]
patch 5 - Make nsRubyFrame inherit nsInlineFrame

>-  // The leadings required to put the annotations.
>+  // The leadings required to put the annotations. They are not
>+  // initialized until the first reflow.
>   nscoord mBStartLeading;
>   nscoord mBEndLeading;

This should say "The leading" rather than "The leadings", since leading is a mass noun.



This IsFrameOfType change is good, and reminds me that
nsRubyBaseContainerFrame should also return false for eSupportsCSSTransforms,
like nsInlineFrame and nsRubyTextContainerFrame do.  I filed bug 1121738 for this.  (I suspect it regressed at some previous inheritance change.)
Attachment #8548639 - Flags: review?(dbaron) → review+
Comment on attachment 8548640 [details] [diff] [review]
patch 6 - Fix border/padding of ruby frame

r=dbaron, although I wonder if the nsCSSRendering code needs to handle some other frame types as well.
Attachment #8548640 - Flags: review?(dbaron) → review+
Attachment #8548641 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #48)
> Comment on attachment 8548640 [details] [diff] [review]
> patch 6 - Fix border/padding of ruby frame
> 
> r=dbaron, although I wonder if the nsCSSRendering code needs to handle some
> other frame types as well.

I was using static_cast<nsInlineFrame*>(do_QueryFrame()), but I guess it might affect performance, and I don't see other subclasses of nsInlineFrame actually need this, so finally I changed it to this check.
(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #49)
> (In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from
> comment #48)
> > Comment on attachment 8548640 [details] [diff] [review]
> > patch 6 - Fix border/padding of ruby frame
> > 
> > r=dbaron, although I wonder if the nsCSSRendering code needs to handle some
> > other frame types as well.
> 
> I was using static_cast<nsInlineFrame*>(do_QueryFrame()), but I guess it
> might affect performance, and I don't see other subclasses of nsInlineFrame
> actually need this, so finally I changed it to this check.

Probably best to use either the do_QueryFrame test in both places, or maybe even use IsFrameOfType(eLineParticipant).  (I think what it's trying to test is whether the split boxes join logically in the inline or block directions, which makes me think eLineParticipant might be best.)
Change that to using IsFrameOfType(eLineParticipant) might need a little more work than do_QueryFrame, since the function it calls assume the frame is a inline frame.

I'll make it use do_QueryFrame instead. I still suspect that it might cause some performance regression, though.
Blocks: 1135954
You need to log in before you can comment on or make changes to this bug.