1117227 - text-overflow doesn't work in vertical writing mode

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Jonathan Kew [:jfkthame]]

data:text/html,
  <div style="writing-mode:vertical-lr; height:100px;
              white-space:pre;overflow:hidden;
              text-overflow:ellipsis">the quick brown fox

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

When I look into it more deeply, I found it is a big project which is not limited to TextOverflow files, but also related to nsTextFrame's decoration drawing, which I'm not sure whether you are working on. So drop for now.

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 1 - Add IntersectRect method to LogicalRect

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 2 - Convert TextOverflow to logical coordinate

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 3 - Make nsTextFrame::MeasureCharClippedText use isize

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 4 - Fix baseline of text-overflow marker

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 5 - Fix orientation of ellipsis

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 6 - Reftest

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d65c1c62c1df

Comment 9

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

Comment on attachment 8596322 [details] [diff] [review]
patch 2 - Convert TextOverflow to logical coordinate

Review of attachment 8596322 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/TextOverflow.h
@@ +110,4 @@
>    };
>  
> +  mozilla::LogicalRect
> +    GetLogicalScrollableOverflowRectToBlock(nsIFrame* aFrame) const

GetLogicalScrollableOverflowRectRelativeToBlock

@@ +146,5 @@
>     *   inline-level frames that are clipped by the current marker width
>     */
>    void ExamineFrameSubtree(nsIFrame*       aFrame,
> +                           const mozilla::LogicalRect& aContentArea,
> +                           const mozilla::LogicalRect& aInsideMarkersArea,

Put a typedef mozilla::LogicalRect LogicalRect; in TextOverflow to avoid too many mozilla:: prefixes

Comment 10

[Jonathan Kew [:jfkthame]]

Comment on attachment 8596322 [details] [diff] [review]
patch 2 - Convert TextOverflow to logical coordinate

Review of attachment 8596322 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/TextOverflow.cpp
@@ +97,5 @@
>           f->GetType() != nsGkAtoms::scrollFrame) {
>      f = f->GetParent();
>    }
> +  auto overflow = aFrame->GetWritingMode().IsVertical() ?
> +    f->StyleDisplay()->mOverflowY : f->StyleDisplay()->mOverflowX;

This looks like it'll crash trying to get StyleDisplay() if f is null. (It used to be protected by the !f test.)

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Jonathan Kew (:jfkthame) from comment #10)
> Comment on attachment 8596322 [details] [diff] [review]
> patch 2 - Convert TextOverflow to logical coordinate
> 
> Review of attachment 8596322 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/generic/TextOverflow.cpp
> @@ +97,5 @@
> >           f->GetType() != nsGkAtoms::scrollFrame) {
> >      f = f->GetParent();
> >    }
> > +  auto overflow = aFrame->GetWritingMode().IsVertical() ?
> > +    f->StyleDisplay()->mOverflowY : f->StyleDisplay()->mOverflowX;
> 
> This looks like it'll crash trying to get StyleDisplay() if f is null. (It
> used to be protected by the !f test.)

Good catch!

Comment 12

[Jonathan Kew [:jfkthame]]

Comment on attachment 8596321 [details] [diff] [review]
patch 1 - Add IntersectRect method to LogicalRect

Review of attachment 8596321 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/WritingModes.h
@@ +1732,5 @@
>    }
>  
> +  /**
> +   * Set *this to be the rectangle containing the intersection of aRect1
> +   * and aRect2, return whether the intersection is empty.

s/empty/non-empty/

Comment 13

[Jonathan Kew [:jfkthame]]

Comment on attachment 8596326 [details] [diff] [review]
patch 5 - Fix orientation of ellipsis

Review of attachment 8596326 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the mask fixed, see below.

::: gfx/thebes/gfxTextRun.cpp
@@ +2580,2 @@
>      if (mCachedEllipsisTextRun &&
> +        (mCachedEllipsisTextRun->GetFlags() & ~TEXT_ORIENT_MASK) == aFlags &&

I don't think you really wanted to apply ~ to TEXT_ORIENT_MASK here!

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Jonathan Kew (:jfkthame) from comment #13)
> Comment on attachment 8596326 [details] [diff] [review]
> patch 5 - Fix orientation of ellipsis
> 
> Review of attachment 8596326 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the mask fixed, see below.
> 
> ::: gfx/thebes/gfxTextRun.cpp
> @@ +2580,2 @@
> >      if (mCachedEllipsisTextRun &&
> > +        (mCachedEllipsisTextRun->GetFlags() & ~TEXT_ORIENT_MASK) == aFlags &&
> 
> I don't think you really wanted to apply ~ to TEXT_ORIENT_MASK here!

Ahh, silly issue... I think initially I wanted to write "& ~TEXT_IS_PERSISTENT", then I changed my mind to "& TEXT_ORIENT_MASK", then it became this...

So which one do you think is better?

Comment 15

[Jonathan Kew [:jfkthame]]

I think I prefer "& TEXT_ORIENT_MASK" here. It's easy to relate to the assertion just above, which makes it clear no other flags are supposed to be passed; so we're only interested in this field of the flags word.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b76fcf24a2f7
https://hg.mozilla.org/integration/mozilla-inbound/rev/303979c9c80d
https://hg.mozilla.org/integration/mozilla-inbound/rev/34d73ad5090a
https://hg.mozilla.org/integration/mozilla-inbound/rev/d33657df2b28
https://hg.mozilla.org/integration/mozilla-inbound/rev/18d118c05f8a
https://hg.mozilla.org/integration/mozilla-inbound/rev/58f1b4f600d4

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b76fcf24a2f7
https://hg.mozilla.org/mozilla-central/rev/303979c9c80d
https://hg.mozilla.org/mozilla-central/rev/34d73ad5090a
https://hg.mozilla.org/mozilla-central/rev/d33657df2b28
https://hg.mozilla.org/mozilla-central/rev/18d118c05f8a
https://hg.mozilla.org/mozilla-central/rev/58f1b4f600d4