Closed Bug 1117227 Opened 5 years ago Closed 5 years ago

text-overflow doesn't work in vertical writing mode

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: jfkthame, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

Attachments

(6 files)

data:text/html,
  <div style="writing-mode:vertical-lr; height:100px;
              white-space:pre;overflow:hidden;
              text-overflow:ellipsis">the quick brown fox
Assignee: nobody → quanxunzhen
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.
Assignee: quanxunzhen → nobody
Assignee: nobody → quanxunzhen
Attachment #8596326 - Flags: review?(jfkthame)
Attachment #8596328 - Flags: review?(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.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
Attachment #8596322 - Flags: review?(roc) → review+
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.)
(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 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/
Attachment #8596321 - Flags: review?(jfkthame) → review+
Attachment #8596325 - Flags: review?(jfkthame) → review+
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!
Attachment #8596326 - Flags: review?(jfkthame) → review+
(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?
Attachment #8596328 - Flags: review?(jfkthame) → review+
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.
Blocks: 1157951
You need to log in before you can comment on or make changes to this bug.