Last Comment Bug 684266 - text-overflow: ellipsis with negative text-indent but padding bigger than text-indent shows ellipsis at beginning
: text-overflow: ellipsis with negative text-indent but padding bigger than tex...
Status: VERIFIED FIXED
[inbound][qa!]
: dev-doc-complete, verified-aurora, verified-beta
Product: Core
Classification: Components
Component: Layout: Block and Inline (show other bugs)
: 9 Branch
: All All
: -- normal (vote)
: mozilla10
Assigned To: Mats Palmgren (:mats)
:
: Jet Villegas (:jet)
Mentors:
: 689912 (view as bug list)
Depends on:
Blocks: 312156 669579 677582 690131
  Show dependency treegraph
 
Reported: 2011-09-02 09:29 PDT by Piotr Duda
Modified: 2011-12-19 13:15 PST (History)
19 users (show)
mats: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed


Attachments
test.html (240 bytes, text/html)
2011-09-02 09:29 PDT, Piotr Duda
no flags Details
test without overflow:ellipsis (245 bytes, text/html)
2011-09-21 02:20 PDT, :aceman
no flags Details
trunk patch (24.25 KB, patch)
2011-10-04 09:49 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
branch patch (35.10 KB, patch)
2011-10-04 09:58 PDT, Mats Palmgren (:mats)
roc: review+
Details | Diff | Splinter Review
trunk patch, rev 2 (32.99 KB, patch)
2011-10-05 14:29 PDT, Mats Palmgren (:mats)
roc: review+
Details | Diff | Splinter Review
branch patch, v2 (35.13 KB, patch)
2011-10-05 14:35 PDT, Mats Palmgren (:mats)
roc: review+
blizzard: approval‑mozilla‑beta+
Details | Diff | Splinter Review
trunk patch, rev 2, part 1 (32.41 KB, patch)
2011-10-06 04:31 PDT, Mats Palmgren (:mats)
bzbarsky: review+
blizzard: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
trunk patch, rev 2, part 2 (1.73 KB, patch)
2011-10-06 04:32 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review

Description Piotr Duda 2011-09-02 09:29:24 PDT
Created attachment 557859 [details]
test.html

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:6.0.1) Gecko/20100101 Firefox/6.0.1
Build ID: 20110830214506

Steps to reproduce:

Created simple html (see attached test.html) with div contaning text "blah blah blah" stylled with:
  text-overflow: ellipsis;
  padding-left: 40px;
  text-indent: -35px;
  overflow: hidden;  



Actual results:

Firefox clips text and shows ellipsis at begginning


Expected results:

Text should not be clipped.
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2011-09-02 10:02:04 PDT
The text _is_ in fact overflowing... but maybe we should be looking at overflow past the padding edge, not past the content edge?  roc?  mats?
Comment 2 Piotr Duda 2011-09-02 10:16:08 PDT
I forgot to mention, if "text-overflow: ellipsis" is removed from example, text isn't clipped.
Comment 3 Mats Palmgren (:mats) 2011-09-02 16:51:17 PDT
(In reply to Boris Zbarsky (:bz) from comment #1)
> The text _is_ in fact overflowing... but maybe we should be looking at
> overflow past the padding edge, not past the content edge?

The content edge is what the (editor's draft) spec says and what other UAs implement.
http://dev.w3.org/csswg/css3-ui/#text-overflow

Other UAs don't implement text-overflow on the start-edge (so far), but the spec
says there SHOULD be a marker in that case too.

The reported issue is invalid since we're doing what the spec says.
I don't think we should try to change it to make text-overflow only consider
overflow past the padding edge since UAs are compatible on the current behavior.
We could make text-overflow apply only to the end-edge by default, but that
isn't a very good default in a scrolling situation, and the cases where an
unwanted start-edge marker occurs are quite rare anyway.
So I prefer to keep it as is.

Note: in Firefox 9 you will be able to specify the marker on either side
using the new "text-overflow: <left> <right>" syntax (bug 677582)
Comment 4 Piotr Duda 2011-09-03 01:47:15 PDT
I still consider this as bug because presence of text-overflow: ellipsis should not change rendering if without it text isn't clipped, so either text-overflow implementation/specification is buggy or implementation of text-indent with negative values is buggy.
Comment 5 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-09-03 05:44:43 PDT
Overflow:hidden etc essentially clip to the padding edge (In reply to Mats Palmgren [:mats] from comment #3)
> The content edge is what the (editor's draft) spec says and what other UAs
> implement.
> http://dev.w3.org/csswg/css3-ui/#text-overflow

Where does the spec say that?

Using the padding edge would make more sense to me since clipping/scrolling normally happen at the padding edge for overflow:hidden etc.
Comment 6 :aceman 2011-09-21 02:20:35 PDT
Created attachment 561415 [details]
test without overflow:ellipsis
Comment 7 :aceman 2011-09-21 02:35:16 PDT
I can confirm the problem in comment 4. In Firefox 6, there is no support for text-overflow: ellipsis, it does not clip the text, it seems to be using the padding edge.
In Firefox 9, it behaves the same if there is no text-overflow: ellipsis (no clip). If overflow is specified, the text is clipped at content edge and ellipsis on the left is shown. Shouldn't text-overflow just control whether the 3 dots are shown and not affect where the text clips? Where does the spec say when text-overflow:ellipsis is on (and not clip), edges chosen by overflow: hidden are to be disregarded? It seems clip and ellipsis should use the same edge.
Comment 8 Ioana (away) 2011-09-22 05:45:14 PDT
Verified this issue on several builds using the test case attached by the reporter.

The issue doesn't reproduce on releases 5.0 and older and on 6.0.2 but it does reproduce on 6.0 and on the latest beta, aurora and nightly (Build ID: 20110921030906).
Comment 9 :aceman 2011-09-22 05:59:12 PDT
Are you sure you see the ellipsis on 6.0? I think it should not be supported until 7.0.
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2011-09-28 07:53:38 PDT
> Where does the spec say that?

It doesn't seem to, actually.

> and what other UAs implement

_This_ is true.  The only issue is that other UAs don't implement ellipsing on the left.  Testcase:

  data:text/html,<div style="text-overflow: ellipsis; overflow: hidden; white-space: pre; width: 100px; padding-right: 500px">Some text and more text</div>

I think I agree with Robert here: using the same edge (padding edge) for both text-overflow and regular overflow makes the most sense.  The only issue, I think, is that this means placing the ellipsis in the padding, right?
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2011-09-28 07:57:20 PDT
The other option is to accept that text-overflow has been hopelessly poisoned, only do overflow on the right for it, and add a new property for doing the right thing....  We're getting a fair number of compat issues from sites that overflow on the left, that overflow inline-blocks, etc....  Specifying something that doesn't match shipping implementations strikes again.  :(
Comment 12 Mats Palmgren (:mats) 2011-09-28 09:09:01 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5)
> Where does the spec say that?

http://dev.w3.org/csswg/css3-ui/#text-overflow
First sentence:
"This property specifies rendering when inline content overflows its block container element..."

Overflow conceptually occurs at the content edge, right? then it's rendered over the
padding area and then clipped at the padding edge.
http://www.w3.org/TR/CSS21/visufx.html#overflow-clipping

Later in http://dev.w3.org/csswg/css3-ui/#text-overflow
"implementations must hide characters  [...] as necessary to fit the ellipsis"

which I read as "to fit the ellipsis in the content box (so that no overflow occurs)".
But yeah, I agree the spec could be clearer on this point.

Firefox, Chrome, Opera and IE are compatible on inserting the ellipsis
inside the content box.  The reported issue only occurs because we do
ellipsing on both sides by default.  I thought we agreed on doing that
in both scrolling and non-scrolling situations was better, for consistency.
But Tantek says in bug 688878 comment 25 that ellipsing the start edge
should only occur when scrolling.
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2011-09-28 09:13:34 PDT
> Overflow conceptually occurs at the content edge, right?

The distinction hasn't mattered until now, given the painting over the padding area bit.

> But Tantek says in bug 688878 comment 25 that ellipsing the start edge
> should only occur when scrolling.

That's just because he didn't realize there was actual overflow to the left in that situation, as far as I can tell.

Anyway, my general opinion on all this is in comment 11.
Comment 14 Tantek Çelik 2011-09-28 10:31:38 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5)
> Overflow:hidden etc essentially clip to the padding edge (In reply to Mats
> Palmgren [:mats] from comment #3)
> > The content edge is what the (editor's draft) spec says and what other UAs
> > implement.
> > http://dev.w3.org/csswg/css3-ui/#text-overflow
> 
> Where does the spec say that?
> 
> Using the padding edge would make more sense to me since clipping/scrolling
> normally happen at the padding edge for overflow:hidden etc.

Agreed with RoC, Boris and Piotr.

If the "overflow:hidden" by itself is not clipping any characters then it's a bug if text-overflow changes the rendering. Nothing clipped means nothing should be ellipsed.

"using the same edge (padding edge) for both text-overflow and regular overflow makes the most sense. "

I'd like to try that change (in spec and implementation) as our Plan A on this and see if it fixes the "fair number of compat issues from sites that overflow on the left". If not, see below for Plan B.

(In reply to Mats Palmgren [:mats] from comment #12)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5)
> > Where does the spec say that?
> 
> http://dev.w3.org/csswg/css3-ui/#text-overflow
> First sentence:
> "This property specifies rendering when inline content overflows its block
> container element..."
> 
> Overflow conceptually occurs at the content edge, right? then it's rendered
> over the
> padding area and then clipped at the padding edge.
> http://www.w3.org/TR/CSS21/visufx.html#overflow-clipping

Interesting distinction in the spec, but in practice, content doesn't appear to overflow unless it goes beyond the padding box (whereupon it is clipped by overflow:hidden).

> Later in http://dev.w3.org/csswg/css3-ui/#text-overflow
> "implementations must hide characters  [...] as necessary to fit the
> ellipsis"
> 
> which I read as "to fit the ellipsis in the content box (so that no overflow
> occurs)".
> But yeah, I agree the spec could be clearer on this point.

Agreed, the spec could be clearer, and I think the best to match expectations would be to change to explicitly say "to fit the ellipsis in the padding box (so that no characters are clipped)"

> Firefox, Chrome, Opera and IE are compatible on inserting the ellipsis
> inside the content box.

Is this on the right? So if overflow:hidden doesn't clip any characters on the right (i.e. characters being rendering on the right padding area), then they still ellipse those characters out? That would violate visual expectations, and in fact, I think it would be a *better* text-overflow implementation to only ellipse characters on the right as well when overflow:hidden is clipping them out.


> The reported issue only occurs because we do
> ellipsing on both sides by default.  I thought we agreed on doing that
> in both scrolling and non-scrolling situations was better, for consistency.
> But Tantek says in bug 688878 comment 25 that ellipsing the start edge
> should only occur when scrolling.

The spec only discusses the situation when scrolling, however I see your point about consistency.

When fantasai and I were mocking up / specifying the left-side ellipsing functionality, we specifically wanted to avoid unintended left-side ellipses in normal flow and thus noted the scrolling scenario in particular.


(In reply to Boris Zbarsky (:bz) from comment #11)
> The other option is to accept that text-overflow has been hopelessly
> poisoned,

I don't think it's hopeless, rather perhaps we were too optimistic to think that specifying a single value could automatically work for both left and right edges.

> only do overflow on the right for it, and add a new property for
> doing the right thing....

We don't need a new property. 

Plan B:

We could treat the single value case as interoperably implemented by others for years, which is only on the right edge.

text-overflow: ellipsis; /* ellipse at right edge, treat left edge as hidden */
text-overflow: ellipsis ellipsis; /* ellipse at left and right edges */

But again, I don't want to give-up just yet. Let's fix the ellipsing to occur only when overflow:hidden would otherwise clip characters (visual effect leads to visual effect), and see if that's sufficient. If not, *then* we should consider this option.
Comment 15 Boris Zbarsky [:bz] (still a bit busy) 2011-09-28 10:43:03 PDT
> Is this on the right?  So if overflow:hidden doesn't clip any characters on the right
> (i.e. characters being rendering on the right padding area), then they still ellipse
> those characters out?

Yes.  Did you try the testcase in comment 10?  I haven't tested IE, but both WebKit and Presto do the same thing on it as Gecko right now.

> That would violate visual expectations

Sure, but it's what UAs do.  Will changing them break sites?  Maybe.  Not as likely before we shipped, because people weren't using text-overflow as much, but at this point, all major UAs will all have interoperable behavior in final releases here for at least 3 months before anything changes.  How many sites will come to rely on that behavior during that time?

Yes, this is why vendor prefixes exist for pre-CR specs.  :(

> I don't think it's hopeless

I see a spec that does not match _any_ of the implementations, on purpose, in fundamental ways.  How is that not a hopeless position, pray tell me?

This is not limited to left overflow and the choice of padding vs content edge.  There are commonly deployed webmail clients that were relying on text-overflow not ellipsing inline-blocks.  They're working fixing their code, and then have to get the update rolled out to all the installs, but in the meantime our users can't read their e-mail... 

> We could treat the single value case as interoperably implemented by others for years,
> which is only on the right edge.

Which edge?  Padding or content?

I'm starting to think that we should have disabled text-overflow on beta once bug 680610 was encountered....  That's water under the bridge, but I'd like to figure out how we can recover from this all without hurting users and web developers even more.
Comment 16 Tantek Çelik 2011-09-28 11:17:15 PDT
(In reply to Boris Zbarsky (:bz) from comment #15)
> > Is this on the right?  So if overflow:hidden doesn't clip any characters on the right
> > (i.e. characters being rendering on the right padding area), then they still ellipse
> > those characters out?
> 
> Yes.  Did you try the testcase in comment 10?  I haven't tested IE, but both
> WebKit and Presto do the same thing on it as Gecko right now.

Ah just saw the data: URL - thanks.


> > That would violate visual expectations
> 
> Sure, but it's what UAs do.  Will changing them break sites?  Maybe.

I'm going to say no, because FF6 and below would simply render the characters into the padding, thus if that were a problem - that would have been reported.  

Changing that to *only* ellipse the right edge characters that were being clipped by overflow:hidden will only change the rendering to show an ellipsis inside the padding-box where otherwise you would have seen clipped characters. I don't see how that would break sites that were ok with characters being clipped.


> Not as
> likely before we shipped, because people weren't using text-overflow as
> much, but at this point, all major UAs will all have interoperable behavior
> in final releases here for at least 3 months before anything changes.  How
> many sites will come to rely on that behavior during that time?

Probably depends on how much we and others evangelize people to use text-overflow. If we communicate it as, we have a first-cut implementation of text-overflow in FF7, try it out, but we're still working out some bugs (true per bugzilla), in those 3 months people will be fairly cautious.

> Yes, this is why vendor prefixes exist for pre-CR specs.  :(

I don't think anyone is disagreeing here - are you saying we should have shipped this as -moz-text-overflow despite every other browser shipping it unprefixed?
(not a bad assertion if you are saying that, and perhaps lesson learned for future apparently interoperable unprefixed pre-CR properties)


> > I don't think it's hopeless
> 
> I see a spec that does not match _any_ of the implementations, on purpose,
> in fundamental ways.  How is that not a hopeless position, pray tell me?

The musts match existing implementations. The shoulds (e.g. scrolling / ellipsis interaction) match what Fantasai, Roc, and I believe to be more ideal behavior that browsers can implement without breaking content.

In addition, the two value syntax (as well as the string values) are "at risk" in the spec and deliberately so. That's how we capture things in the spec that implementations may not match yet but at least somewhat intend to.


> This is not limited to left overflow and the choice of padding vs content
> edge.  There are commonly deployed webmail clients that were relying on
> text-overflow not ellipsing inline-blocks.

Are these captured as test-cases and bug #s at least for tracking purposes? (beyond Roundcube)


> > We could treat the single value case as interoperably implemented by others for years,
> > which is only on the right edge.
> 
> Which edge?  Padding or content?

Padding. Per the reasoning in previous comments.


> I'm starting to think that we should have disabled text-overflow on beta
> once bug 680610 was encountered... 

Just saw this. Yes, I'd tend to agree with that your sentiment that that's what beta testing is for - to act on such reported issues. At a minimum a bug like that would show good reason to -moz- prefix *our* text-overflow implementation until we iterated (spec/implementation) and resolved the issue(s).


> That's water under the bridge,

I think it's good data on the interaction of our rapid beta/release schedule and adding new CSS features and we should keep it in mind for future features.


> but I'd
> like to figure out how we can recover from this all without hurting users
> and web developers even more.

Agreed. I still think we can both recover from this, and still provide a better text-overflow experience (padding edge).

I can take care of speccing it such that such variance (content edge vs padding edge) is permitted in a strict "must" reading, but the more ideal visual behavior (padding edge) is preferred per a "should". We've done this with other CSS features, and it's how we advance the spec and implementations without either breaking things or being locked into bugwards compat with suboptimal behavior.
Comment 17 Boris Zbarsky [:bz] (still a bit busy) 2011-09-28 11:28:01 PDT
> are you saying we should have shipped this as -moz-text-overflow

I'm torn on that.  I know why we didn't, but shipping something with very different behavior under the same name is just .... subtly wrong.  On the other hand, I doubt we would ever have been able to unprefix if we did ship prefixed in this case.

I'm ok with trying your plan A and seeing how it goes, esp. if we can get it into beta ASAP.  Mats, thoughts?
Comment 18 David Baron :dbaron: ⌚️UTC-10 2011-09-28 11:39:36 PDT
Isn't changing to the padding edge going to make us even less compatible with other implementations, even if it happens to fix a few testcases?
Comment 19 Boris Zbarsky [:bz] (still a bit busy) 2011-09-28 11:41:24 PDT
Yes, that's the primary argument against it.  The question is whether the incompatibility will break sites in practice...
Comment 20 Tantek Çelik 2011-09-28 11:57:22 PDT
(In reply to David Baron [:dbaron] from comment #18)
> Isn't changing to the padding edge going to make us even less compatible
> with other implementations, even if it happens to fix a few testcases?

dbaron per comment 16: since FF6 and below would simply render the characters into the padding, thus if that were a problem - that would have been reported.  

Changing that to *only* ellipse the right edge characters that were being clipped by overflow:hidden will only change the rendering to show an ellipsis inside the padding-box where otherwise you would have seen clipped characters. I don't see how that would break sites that were ok with characters being clipped.

I think this is one of those cases where there is an opportunity to provide a better (and more visually expected/predictable) implementation.

So no, it's not "even less compatible", certainly not compared to FF6 and below.
Comment 21 Mats Palmgren (:mats) 2011-09-28 12:29:11 PDT
I don't like plan A - all UAs are compatible on using the content edge.
We should at least have other UA vendors agree on changing to the
padding edge before we do so.

Plan B sounds good to me, it makes us more compatible with other UAs.
(and I assume that Tantek means that the single value is the *end*
edge rather than the right edge)

One could also imagine a variation of plan B where we suppress ellipsing
on the start edge only when the scrollbar is at the start position.
That is, ellipsing occurs on both edges while scrolling, but not by negative
text-indent and such.
Comment 22 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-09-28 13:46:38 PDT
I like plan B. Let's do it.
Comment 23 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-09-28 13:50:12 PDT
Also, we need to make all these text-overflow bugs block bug 312156 so it's easier to see all the issues.
Comment 24 fantasai 2011-09-28 14:48:16 PDT
I think you should definitely not ellipsize to padding edge. To do so for the end edge would be wrong per desired behavior (what is the point of padding if not to provide visual separation between the content and the border?); to do so for the start edge would be inconsistent.

You could trigger ellipsis if there's overflow out the padding edge, but the place/clip to the content edge once there is overflow. I guess that's a Plan C.

Either that or Plan B (making the single value apply to the end edge only) makes sense to me. I do think this discussion should be transferred to www-style, however. We might be the only ones thinking about this problem right now, but we aren't the only ones who will have to implement the spec changes.
Comment 25 Tantek Çelik 2011-09-28 15:58:57 PDT
Ok it sounds like we have a rough consensus on Plan B so let's move forward with that.

I'll edit the spec accordingly, and ...

(In reply to fantasai from comment #24)
> I do think this discussion should be transferred to
> www-style, however. We might be the only ones thinking about this problem
> right now, but we aren't the only ones who will have to implement the spec
> changes.

I'll follow-up on www-style pointing out the change, the experience/reasoning, and this bug.
Comment 26 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-09-28 16:28:12 PDT
Also, based on the bugs people are reporting about replaced elements causing ellipses on Firefox but not other browsers, we should consider changing the spec (and our implementation) to not do that. Not really this bug though...
Comment 27 David Baron :dbaron: ⌚️UTC-10 2011-09-28 19:02:59 PDT
Another alternative:  never create an ellipsis at an edge where the element with 'overflow' can't be scrolled any more to that side.  In other words, elements in LTR start out scrolled to top-left, so at that initial scroll position there would never be an ellipsis on the left side.


Is plan B to do *right* edge always, even in RTL?  Is that what other browsers implement?  (I seem to remember other browsers not agreeing once RTL was involved.)
Comment 28 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-09-28 19:22:52 PDT
(In reply to David Baron [:dbaron] from comment #27)
> Another alternative:  never create an ellipsis at an edge where the element
> with 'overflow' can't be scrolled any more to that side.  In other words,
> elements in LTR start out scrolled to top-left, so at that initial scroll
> position there would never be an ellipsis on the left side.

We could do that as well as plan B.

> Is plan B to do *right* edge always, even in RTL?  Is that what other
> browsers implement?  (I seem to remember other browsers not agreeing once
> RTL was involved.)

Chrome and IE9 put the ellipsis on the left in RTL. Opera seems to just break (allocates space for ellipsis but doesn't draw it). So I think making a single value apply only to the *end edge* is interoperable (and useful).
Comment 29 Tantek Çelik 2011-09-29 00:53:32 PDT
Thanks Mats, dbaron, roc - yes, one value = applies to end line edge makes sense.

I presume nothing has changed with respect to everyone's preference to keep the two value version "visual" (applies to left and right per everything mats quoted) rather than "logical" (applies to start, end) ?

In which case, that does make the behavior of the property quite different between the one value (text direction relative) and two value (absolute visual) cases.
Comment 30 Mats Palmgren (:mats) 2011-10-04 09:49:57 PDT
Created attachment 564589 [details] [diff] [review]
trunk patch

dbaron: please review the style system changes.

I'm adding a nsStyleTextOverflow::mSingleValue that is true when a single
value was specified.
Comment 31 Mats Palmgren (:mats) 2011-10-04 09:55:00 PDT
Comment on attachment 564589 [details] [diff] [review]
trunk patch

roc: please review the layout changes.

I'm using the mSingleValue bit in the style to suppress ellipsing
on the start side.  I'm also suppressing ellipsing if there's nothing
to scroll to on that side (may be both sides).
Comment 32 Mats Palmgren (:mats) 2011-10-04 09:58:16 PDT
Created attachment 564594 [details] [diff] [review]
branch patch

This patch is for branches that doesn't have the two-value syntax
(Fx8 and earlier).  The layout code is the same except there is
no mSingleValue bit to check.
Comment 33 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-04 12:44:32 PDT
Comment on attachment 564589 [details] [diff] [review]
trunk patch

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

::: layout/style/nsStyleStruct.h
@@ +1157,5 @@
>    }
>  
>    nsStyleTextOverflowSide mLeft;
>    nsStyleTextOverflowSide mRight;
> +  bool mSingleValue;

I think this should be "mLogicalDirections" so that when set, mLeft is the start direction and mRight is the end direction. This is a bit more flexible and avoids having layout depend on the parsing rule.

Then I suggest you add GetLeft/GetRight methods on nsStyleTextOverflow that take the direction as a parameter, and apply the direction internally if necessary, always returning actual left and right nsStyleTextOverflowSide values.
Comment 34 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-04 12:46:53 PDT
Comment on attachment 564594 [details] [diff] [review]
branch patch

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

r+ with that.

::: layout/generic/TextOverflow.cpp
@@ +408,5 @@
> +  bool suppressRight =
> +    mRight.mStyle->mType == NS_STYLE_TEXT_OVERFLOW_CLIP || mBlockIsRTL;
> +  nsIScrollableFrame* scroll = nsLayoutUtils::GetScrollableFrameFor(mBlock);
> +  if (scroll &&
> +      (scroll->GetScrollbarVisibility() & nsIScrollableFrame::HORIZONTAL)) {

I don't think checking scrollbar visibility is desired here. we should just check the range.
Comment 35 Mats Palmgren (:mats) 2011-10-05 14:29:03 PDT
Created attachment 565018 [details] [diff] [review]
trunk patch, rev 2

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #34)
> > +  nsIScrollableFrame* scroll = nsLayoutUtils::GetScrollableFrameFor(mBlock);
> > +  if (scroll &&
> > +      (scroll->GetScrollbarVisibility() & nsIScrollableFrame::HORIZONTAL)) {
>
> I don't think checking scrollbar visibility is desired here. we should just
> check the range.

We need to exclude overflow:hidden though since it also creates a
nsIScrollableFrame.  The TextOverflow object already has a suitable bit
for this so that's what I used.  Note that not checking scrollbar
visibility brings a subtle change to overflow:auto blocks when there's
no scrollbar - previously it did ellipsing and now it don't.
I think this is what makes sense because if the overflow doesn't trigger
a scrollbar it's not "scrollable overflow" and shouldn't trigger
ellipsing -- same as when the scrollbar is scrolled to the end position.
(I have added tests for this case)
Comment 36 Mats Palmgren (:mats) 2011-10-05 14:35:29 PDT
Created attachment 565021 [details] [diff] [review]
branch patch, v2

Updated the nsIScrollableFrame section the same way as
described above.  It doesn't seem worth messing with the
style system to abstract the left/right parts in this
case though since it's always the same value.
Comment 37 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-05 16:22:48 PDT
Comment on attachment 565018 [details] [diff] [review]
trunk patch, rev 2

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

Looks good to me, although I think it would make sense to chop this patch into one piece for the change to make one value logical, and another piece for the change to not ellipsize if we're at the edge of the scrollable range.
Comment 38 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-05 16:23:36 PDT
And of course don't forget to request review from dbaron or bz for the style system changes.
Comment 39 Mats Palmgren (:mats) 2011-10-06 04:31:51 PDT
Created attachment 565182 [details] [diff] [review]
trunk patch, rev 2, part 1

Requesting review on the style system changes.
Comment 40 Mats Palmgren (:mats) 2011-10-06 04:32:56 PDT
Created attachment 565183 [details] [diff] [review]
trunk patch, rev 2, part 2
Comment 41 Mats Palmgren (:mats) 2011-10-11 14:53:20 PDT
Comment on attachment 565182 [details] [diff] [review]
trunk patch, rev 2, part 1

Requesting review on the style system changes.
Comment 42 Boris Zbarsky [:bz] (still a bit busy) 2011-10-11 20:00:03 PDT
Comment on attachment 565182 [details] [diff] [review]
trunk patch, rev 2, part 1

I'd prefer that GetLeft/GetRight return const references, not pointers.

Document in nsStyleTextOverflow that mLogicalDirections is true if and only if both values were specified?

r=me with those nits.
Comment 43 Mats Palmgren (:mats) 2011-10-11 20:25:22 PDT
> I'd prefer that GetLeft/GetRight return const references, not pointers.

Fine with me if roc is ok with that.  roc?

> Document in nsStyleTextOverflow that mLogicalDirections is true if and only
> if both values were specified?

It's true when only one value was specified.  I'll document it.
Comment 44 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-11 21:14:35 PDT
(In reply to Mats Palmgren [:mats] from comment #43)
> > I'd prefer that GetLeft/GetRight return const references, not pointers.
> 
> Fine with me if roc is ok with that.  roc?

Sure.
Comment 47 Mats Palmgren (:mats) 2011-10-13 08:17:14 PDT
Comment on attachment 565182 [details] [diff] [review]
trunk patch, rev 2, part 1

All parts of "trunk patch" applies to Fx9.
Comment 48 Mats Palmgren (:mats) 2011-10-13 08:19:23 PDT
Comment on attachment 565021 [details] [diff] [review]
branch patch, v2

Applies to Fx8 (and Fx7 if you want it)
Comment 49 Christopher Blizzard (:blizzard) 2011-10-13 14:55:00 PDT
Comment on attachment 565021 [details] [diff] [review]
branch patch, v2

Approved for the beta branch.  Please land as soon as possible so we can test.
Comment 50 Christopher Blizzard (:blizzard) 2011-10-13 14:55:31 PDT
Comment on attachment 565182 [details] [diff] [review]
trunk patch, rev 2, part 1

Approved for the Aurora branch.  Please land as soon as possible so we can get testing.
Comment 51 Christopher Blizzard (:blizzard) 2011-10-13 14:57:12 PDT
For docs: Need to note that we only affect the right edge in the single text-overflow: value case to remain compatible with other UAs.
Comment 52 Mats Palmgren (:mats) 2011-10-13 15:12:59 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/1406e0d0731b
Comment 54 j.j. 2011-10-14 08:55:50 PDT
*** Bug 689912 has been marked as a duplicate of this bug. ***
Comment 55 Kevin Brosnan [:kbrosnan] 2011-10-20 18:08:16 PDT
Verified Mozilla/5.0 (Android; Linux armv7l; rv:8.0) Gecko/20111019 Firefox/8.0 Fennec/8.0 ID:20111019121950
Comment 56 :aceman 2011-10-21 02:04:52 PDT
Verified on FF10, ID 20111020031025.
Both testcases now do not have ellipsis.
Comment 57 Simona B [:simonab ] 2011-10-21 02:27:47 PDT
Verified issue using the test case attached in the Description - no ellipsis are shown anymore. Verified with the latest Nightly, the latest Aurora and Firefox 8.0b4 on Windows XP, Windows 7, Ubuntu 10.10 and Mac OS X 10.6.

Mozilla/5.0 (Windows NT 5.1; rv:10.0a1) Gecko/20111020 Firefox/10.0a1
Mozilla/5.0 (Windows NT 5.1; rv:9.0a2) Gecko/20111020 Firefox/9.0a2
Mozilla/5.0 (Windows NT 5.1; rv:8.0) Gecko/20100101 Firefox/8.0

Mozilla/5.0 (Windows NT 6.1; rv:10.0a1) Gecko/20111020 Firefox/10.0a1
Mozilla/5.0 (Windows NT 6.1; rv:9.0a2) Gecko/20111020 Firefox/9.0a2
Mozilla/5.0 (Windows NT 6.1; rv:8.0) Gecko/20100101 Firefox/8.0

Mozilla/5.0 (X11; Linux x86_64; rv:10.0a1) Gecko/20111020 Firefox/10.0a1
Mozilla/5.0 (X11; Linux x86_64; rv:9.0a2) Gecko/20111020 Firefox/9.0a2
Mozilla/5.0 (X11; Linux x86_64; rv:8.0) Gecko/20100101 Firefox/8.0

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:10.0a1) Gecko/20111017 Firefox/10.0a1
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:9.0a2) Gecko/20111020 Firefox/9.0a2
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:8.0) Gecko/20100101 Firefox/8.0
Comment 58 Eric Shepherd [:sheppy] 2011-12-19 10:15:37 PST
(In reply to Christopher Blizzard (:blizzard) from comment #51)
> For docs: Need to note that we only affect the right edge in the single
> text-overflow: value case to remain compatible with other UAs.

Say what? (more to the point: there's a lot of discussion here and I'm not sure I know what "affect the right edge in the single text-overflow: value case" refers to).
Comment 59 Mats Palmgren (:mats) 2011-12-19 11:55:12 PST
Given a block that has inline overflow on both horizontal sides...
If you specify only one value (eg. text-overflow:ellipsis;) then ellipsing only
occurs on the *end* edge per the block's direction.  Before this change ellipsing
occurred on both edges.

If you specify two values, (eg. text-overflow:ellipsis ellipsis;), then ellipsing
occurs on both edges, so there's no change in behavior when specifying two values.
Note that the two-value form is still using physical sides,
text-overflow:<left> <right>
Comment 60 Eric Shepherd [:sheppy] 2011-12-19 13:15:26 PST
Documentation updated:

https://developer.mozilla.org/en/CSS/text-overflow#Browser_compatibility

And mentioned on Firefox 10 for developers.

Note You need to log in before you can comment on or make changes to this bug.