Last Comment Bug 690187 - entire text in cell being replaced by ellipsis from text-overflow
: entire text in cell being replaced by ellipsis from text-overflow
Status: RESOLVED FIXED
[inbound]
: regression
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Mats Palmgren (:mats)
:
Mentors:
: 689897 690131 703360 (view as bug list)
Depends on: 712894
Blocks: 312156 680610
  Show dependency treegraph
 
Reported: 2011-09-28 17:56 PDT by Sweeney
Modified: 2012-02-27 17:08 PST (History)
17 users (show)
mats: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
-
-


Attachments
Product Explorer - FF7.png (140.94 KB, image/png)
2011-09-28 17:56 PDT, Sweeney
no flags Details
Previous behaviour under 6.0.2 and earlier (151.06 KB, image/png)
2011-09-28 17:57 PDT, Sweeney
no flags Details
Affected page source (18.35 KB, text/html)
2011-09-28 18:12 PDT, Sweeney
no flags Details
reduced testcase (333 bytes, text/html)
2011-10-14 10:16 PDT, j.j.
no flags Details
Testcase #3 (5.45 KB, text/html)
2011-12-14 21:48 PST, Mats Palmgren (:mats)
no flags Details
part 1, add Marker::mActive bit (9.09 KB, patch)
2011-12-16 22:36 PST, Mats Palmgren (:mats)
roc: review+
Details | Diff | Review
part 2, add aFoundVisibleTextOrAtomic param (8.36 KB, patch)
2011-12-16 22:40 PST, Mats Palmgren (:mats)
roc: review+
Details | Diff | Review
part 3, suppress a marker if it makes the line empty (19.29 KB, patch)
2011-12-16 22:49 PST, Mats Palmgren (:mats)
no flags Details | Diff | Review
part 4, clip or suppress a marker if it makes the line empty (13.93 KB, patch)
2011-12-16 23:08 PST, Mats Palmgren (:mats)
no flags Details | Diff | Review
all the parts as one patch (35.15 KB, patch)
2011-12-16 23:13 PST, Mats Palmgren (:mats)
no flags Details | Diff | Review
Parts 3 and 4 combined (24.44 KB, patch)
2011-12-18 15:36 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details | Diff | Review
part 3, clip or suppress a marker if it makes the line empty (40.46 KB, patch)
2011-12-19 02:30 PST, Mats Palmgren (:mats)
roc: review+
Details | Diff | Review
all the parts as one patch (wdiff) (44.63 KB, patch)
2011-12-19 02:32 PST, Mats Palmgren (:mats)
no flags Details | Diff | Review
simple text-overflow:ellipsis tests for characters, images, inline blocks (16.90 KB, text/html)
2011-12-21 17:57 PST, Tantek Çelik
no flags Details
text-overflow:ellipsis test screenshot in Opera11, Chrome16, Safari5, Firefox8 (278.51 KB, image/png)
2011-12-21 18:14 PST, Tantek Çelik
no flags Details
IE10 screenshot (48.11 KB, image/png)
2011-12-22 13:23 PST, Mats Palmgren (:mats)
no flags Details

Description Sweeney 2011-09-28 17:56:17 PDT
Created attachment 563257 [details]
Product Explorer - FF7.png

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:6.0.2) Gecko/20100101 Firefox/6.0.2
Build ID: 20110902133214

Steps to reproduce:

Firefox automatically upgraded from 6.0.2 to 7.0


Actual results:

Information missing on websites that have apsx pages


Expected results:

No change should have happened
Comment 1 Sweeney 2011-09-28 17:57:29 PDT
Created attachment 563260 [details]
Previous behaviour under 6.0.2 and earlier

Previous behaviour of effected aspx under Firefox 6.0.2 and earlier
Comment 2 Matthias Versen [:Matti] 2011-09-28 18:05:41 PDT
We need either a public URL or a html testcase
Comment 3 Sweeney 2011-09-28 18:12:04 PDT
Created attachment 563262 [details]
Affected page source

Page source of effected page. Page is rendered correct by Firefox 6.0.2,  but information in the tables are not rendered by 7.0 and later.
Comment 4 Sweeney 2011-09-28 18:14:14 PDT
Firefox 6 and earlier not effected by the issue. Firefox 7 and later (including Nightly x64 builds). Not tested on Windows Vista and later.
Comment 5 Alice0775 White 2011-09-29 02:21:10 PDT
It seems that the site uses old spec of text-overflow .
Comment 6 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-10-05 21:22:52 PDT
Mats, do your patches in bug 684266 help here?
Comment 7 j.j. 2011-10-14 08:58:32 PDT
Mozilla/5.0 (Windows NT 5.1; rv:10.0a1) Gecko/20111014 Firefox/10.0a1
NOT fixed with bug 684266
Comment 8 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-10-14 09:45:05 PDT
OK.  Is that because the cells really are overflowing to the right?
Comment 9 j.j. 2011-10-14 10:16:09 PDT
Created attachment 567129 [details]
reduced testcase
Comment 10 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-10-14 10:19:29 PDT
Yeah, that's certainly overflowing.

Tantek, we've now had a number of reports about this being a problem, for both inline-blocks and images....
Comment 11 [:Cww] 2011-10-27 14:35:51 PDT
We're not changing behavior from Firefox 7 to 8 so this we won't track it for 8 but let's track it for 9.  Can we get help looking into it and figuring out next steps?
Comment 12 Sweeney 2011-11-22 16:30:02 PST
The webpage affected by this issue play a big part in our workplace and the issue still affects the latest nightly builds as well. Currently our workstations are locked at Firefox 6.02 to avoid issues introduced in Firefox 7.0
Comment 13 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-11-22 17:18:22 PST
Sweeney, you need to either change the webpage or get Tantek to change the CSS spec on this.  What we're implementing here is exactly what the spec says to do...
Comment 14 Sweeney 2011-11-22 17:28:00 PST
We do not have control over the website in question and the issue has been reported to the company in charge of the website. The issue only affects Firefox (and other Gecko based browsers) , not Chrome or IE (untested with Safari). In the end we may have to move to Chrome (I am avoiding IE) as we have less issues with it and we cannot stay FF 6.02 for security reasons.
Comment 15 Tantek Çelik 2011-11-22 18:30:11 PST
The problem this bug illustrates is what happens when there is only *one* atomic inline-level element and the authoring sloppiness of width:100%; padding: 1px; which will always cause overflow. There's a similar bug (can't find it offhand) of a single image overflowing as well.

The intent of ellipsing is to ellipse, not to eliminate inline content.

The spec currently only hints at this nuance (though can be interpreted otherwise).

Thus in degenerate cases (of only one character / atomic inline-level element), the results can be unexpected and undesirable: elimination of all content.

The spec states:

"... implementations must hide characters and atomic inline-level elements at the applicable edge(s) of the line as necessary to fit the ellipsis/string."

Though it is subtle, the plural forms there "characters" and "elements" provide the hint to a solution, which the spec should clarify (assuming we agree on this).

I am proposing we make this explicit by adding a phrase like:

", after placing at least one character or atomic inline-level element on the line."

to the end of the quoted sentence so that it would read in total as:

"For the ellipsis and string values, implementations must hide characters and atomic inline-level elements at the applicable edge(s) of the line as necessary to fit the ellipsis/string, after placing at least one character or atomic inline-level element on the line."

This clarification will both better reflect the intent of ellipsing, as well as fix bugs like this one that depend on not ellipsing the first (or only!) character / atomic inline-level element in the block with text-overflow ellipsis.

The current spec draft does allow for this behavior (at least one character/element on the line), and thus an update to code and clarification to the spec can proceed asynchronously.

Mats, please chime in on the implementability of this nuance, and Boris/fantasai, please double-check my spec / CSS inline-flow analysis.
Comment 16 fantasai 2011-11-22 22:40:56 PST
I don't see how the existing spec allows this, so you'll have to explain your interpretation in a bit more detail (if that's important).

The proposed text seems fine, but you should also post the issue to www-style so that other people can chime in as to whether that's the desired behavior. (You're not writing a spec for Mozilla, you're writing a spec for the Web, and so deciding whether this is appropriate in a Mozilla bug report is imo inappropriate.)
Comment 17 Tantek Çelik 2011-11-22 23:53:40 PST
Existing spec mentions plurals (characters, elements), not singulars, which are thus not quite covered or are - it's ambiguous, that's my point, and the point of adding the proposed text.

Thanks for your review of the proposed text. 

Mats - awaiting your evaluation of implementability.

Regarding appropriateness (which is OT, but since you (fantasai) brought it up, and I know you too care particularly about such methodological matters), it is both ok, and desirable to discuss ideas/proposals in a smaller group (especially if/when published in the open, like in an open bugzilla bug) to quickly check merits/practicality before taking up everyone's time in a broader forum. I've previously written more about it being ok here: http://tantek.com/2011/168/b1/practices-good-open-web-standards-development#develop-openly . Having thought more about it since, it is actually even *desirable* because it helps filter/prune discussions that might otherwise be a waste of everyone's time. Distributed/clustered (but preferably still openly archived) communications scale better across a broad variety of open standards topics, whether an aspect of a particular CSS property, CSS module, across all of CSS, across visual effects standards, across declarative standards, or the entirety of the open web platform. It's also why we don't discuss all possible ideas on/about all open internet standards on a single mailing list (though the main WHATWG irc and/or list is perhaps the closest counter-attempt).

In this instance, I had you as a peer/colleague double-check my reasoning with respect to spec language. And I'm having mats check its implementability since he's the only one (AFAIK) who's implemented text-overflow richly/thoroughly enough to evaluate the practical effect of this change. If he finds problems, we'll iterate without having to have wasted everyone else's time. If he says ok, then I'll make the editor's draft spec change and also raise it explicitly on www-style as a heads-up to the refinement, with a pointer to this bug and reasoning for it, which will then encourage a broader set of folks to review it for anything that any of us missed. Hopefully we'll even get a quick implementation iteration to check this particular spec update (hypothesis) with a practical example (data) like the one in this bug.
Comment 18 fantasai 2011-11-23 02:24:51 PST
Yeah, sure. I just want to make sure it gets out to www-style for peer-review before we all decide "this is the way it's going to be, let's ship it", and that it actually gets posted there as a discussion item, not "I put it in the spec, nobody said anything negative, silence == consent". Also, don't just post a link to the bug report, summarize the rationale. I hate getting links to Bugzilla on www-style where I have to trawl through all the implementation discussion and patch reviews and study of testcases to find the few sentences worth of design rationale.

Your plural argument still doesn't make any sense to me (are you arguing that you can only hide sequences of multiple characters, not a single character if that's all that's needed to make the ellipsis fit?), but I don't think it matters.
Comment 19 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-11-23 04:45:27 PST
> The issue only affects Firefox (and other Gecko based browsers) , not Chrome or IE

Yeah, because Tantek purposefully wrote the spec to not match WebKit or IE behavior...  which I still think was a mistake.  _Our_ mistake was then implementing that sort of thing blindly.  :(

If this is a managed deployment, could you also deploy a user stylesheet to change the style in this case to work around the problem for now?  If you can deploy a stylesheet at all, I'm happy to talk you through what should go into it offline; please just send me mail.

> I just want to make sure it gets out to www-style for peer-review before we all decide
> "this is the way it's going to be, let's ship it"

I actually don't care about that, for _Mozilla_ shipping it.  Mats, please either implement Tantek's suggestion above or just something entirely compatible with other UAs and let's ship it (ideally on aurora too; beta may be an issue).  We've had enough reports about the current spec causing problems here that I can confidently say it's not compatible with the web, and it's only doing us and web developers harm.

(Note: Mats is on vacation this week, so expect it to be at least until Monday before he responds.)
Comment 20 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-11-23 06:07:37 PST
Actually, on further thought, the suggestion above doesn't help the situation of atomic inlines that are NOT the only child of the block, and we've had multiple reports of problems with that too.

So what _I_ think is that we should just ignore the spec (which imo is broken by design, though perhaps more sane) here and just do something compatible with other UAs.  If there is a desire for a "saner" behavior, it should just be a new value of text-overflow.

I'll post to www-style to this effect.
Comment 21 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-11-23 06:16:13 PST
ccing roc and dbaron too, to make sure they're in the loop on this.
Comment 22 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-11-23 13:25:27 PST
I agree with you. The "saner" behavior has proven to not be Web-compatible, so we need to spec and implement the Web-compatible behavior of clipping atomic inlines.
Comment 23 Tantek Çelik 2011-12-02 15:19:24 PST
(In reply to Boris Zbarsky (:bz) from comment #13)
> get Tantek to change the
> CSS spec on this.
and
(In reply to Boris Zbarsky (:bz) from comment #19)
> Yeah, because Tantek purposefully wrote the spec to not match WebKit or IE
> behavior...  which I still think was a mistake.

Bug-for-bug compat is very low on the priority list for design of CSS features, especially when investigation shows very little compat among existing buggy implementations on details (e.g. i18n, RTL, mixed LTR/RTL, behavior in presence of scrollbars etc. - Elika and I investigated many of these, with a bunch demonstrated by test cases from Roc as well).

While as editor I do have some leeway for specifying details, something as fundamental of what is or is not ellipsed would typically be the scope of a working group discussion and consensus. 

What's in the spec is what's been discussed / agreed to in past working group discussions and meetings, including other browser implementers.

Assume that any/all CSS spec/design issues are discussable in www-style.


(In reply to Boris Zbarsky (:bz) from comment #20)
> Actually, on further thought, the suggestion above doesn't help the
> situation of atomic inlines that are NOT the only child of the block, and
> we've had multiple reports of problems with that too.

Boris, please provide links to the specific bugs you believe document said problems so that we can evaluate them and their applicability objectively, and inform proposed spec changes accordingly.

Thanks.


Mats - any word on implementability of the "clip rather than ellipse when just a single child character or inline atomic element" approach?

If you think that's reasonably easily implementable, I'd like to propose and make that incremental improvement to the spec ASAP.
Comment 24 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-12-02 17:14:51 PST
> especially when investigation shows very little compat among existing buggy
> implementations

There was good compat among existing implementations for atomic inlines, as far as I can tell.

> Assume that any/all CSS spec/design issues are discussable in www-style.

OK.  I should have taken this there months ago.  Next time I'll do that more proactively.

For the rest, every single bug involved is marked as blocking bug 312156 or is marked as a duplicate of one of those.  You may also want to search stackoverflow for recent questions about overflow:ellipsis; there have been at least some there about images being ellipsed when authors did not expect it.  Quite honestly, I'm not going to spend time pointing you yet again to bugs I've already pointed you to in the past and where you ignored the issues.

At least some of those bug reports and questions are not, as I recall, fixed by the "single child inlien" approach.  Furthermore, the "single child inline" thing seems to me to violate the principle of least surprise for authors (e.g. putting in a   will suddenly make the image after it ellipse).  So I don't think we should do that in Gecko.  That's with my layout peer hat on, fwiw.
Comment 25 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-12-03 00:31:12 PST
Boris is right. Please fix the spec.

Whether you do or not, we need to fix our implementation to clip atomic inlines.
Comment 26 Tantek Çelik 2011-12-12 03:37:48 PST
I've analyzed and gone through all of the bugs that bug 312156 "Depends on:" and their duplicates. Of them, here are the bugs related to atomic inlines:

Bug 680610, bug 689897, bug 690131, bug 703360 are additional instances of a single child inline being ellipsed.

I have also searched stackoverflow for recent questions about overflow:ellipsis. Going through the past 2+ months of results, there were no problem reports of treatment of atomic inlines.

As far as I can tell, all of the bugs mentioned above would be fixed by the "single child inline" approach.


However, :bz makes a very good argument that "single child inline" approach violates the principle of least surprise for authors, both in terms of the example given of potentially thin/invisible characters before an inline, and in the case of multiple inlines.

In short, it's a simpler model to always clip at atomic inlines, than to only sometimes (when there's only one) clip at atomic inlines.

I'm going to edit the spec and follow-up with www-style accordingly.
Comment 27 Tantek Çelik 2011-12-12 13:07:08 PST
Spec updated with explicit language that atomic inline elements are to be clipped rather than ellipsed.

http://dev.w3.org/csswg/css3-ui/#text-overflow
Comment 28 christian 2011-12-14 13:58:07 PST
We are tracking this for Fx9...do we still need to? Is there work to be done here or can we ship as-is?
Comment 29 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-12-14 13:59:57 PST
The only work we can do here for Fx9 at this stage is to disable text-overflow:ellipsis support altogether.

I, personally, am tempted to say we should do that until we update to the spec changes.  But it's hard to say...
Comment 30 christian 2011-12-14 14:36:25 PST
We shipped with this behavior in Fx8 though, right?
Comment 31 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-12-14 14:53:54 PST
Yes, and it caused and is causing site breakage.  Think people being unable to use their webmail.
Comment 32 j.j. 2011-12-14 16:43:48 PST
(In reply to Boris Zbarsky (:bz) from comment #31)
> Yes, and it caused and is causing site breakage.  Think people being unable
> to use their webmail.

This was bug 680610, which is fixed on roundcube side. Affected are old roundcube installs but we havn't had new bug reports.
IMHO disabling "text-overflow:ellipsis" in Fx9 would result in outcries of web developers.
Comment 33 j.j. 2011-12-14 17:06:05 PST
... but a fix should land in Fx10 because there are other affected sites out there.
Comment 34 j.j. 2011-12-14 17:15:36 PST
*** Bug 689897 has been marked as a duplicate of this bug. ***
Comment 35 j.j. 2011-12-14 17:15:40 PST
*** Bug 690131 has been marked as a duplicate of this bug. ***
Comment 36 j.j. 2011-12-14 17:15:43 PST
*** Bug 703360 has been marked as a duplicate of this bug. ***
Comment 37 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-12-14 19:59:56 PST
> Affected are old roundcube installs

So "all of them", yes?

> IMHO disabling "text-overflow:ellipsis" in Fx9 would result in outcries of web
> developers.

That's the only thing that makes me just tempted to disable it as opposed to strongly urging that we do so.

> ... but a fix should land in Fx10

I don't see how that can happen.  I don't see how it can land in Fx11.  We're talking invasive changes here.  I'm just really sorry I didn't push harder on this earlier, both in terms of the spec and in terms of our implementation.
Comment 38 j.j. 2011-12-14 20:42:06 PST
> > Affected are old roundcube installs
> So "all of them", yes?

No, the "inline-block" style was added for roundcube 0.5, released 2011-01-12.
Fixed in version 0.6, released 2011-09-28, just in time with the affected Firefox 7.
(http://trac.roundcube.net/ticket/1488061)
Comment 39 Mats Palmgren (:mats) 2011-12-14 21:16:28 PST
I don't think clipping atomic inlines is the best option.
Removing atomic inlines occurs in Fx, IE and Opera - only webkit doesn't.
The parity in Fx is that we do it also for the last character/atomic on
the line.

So my suggestion is that we always leave at least one character/atomic
on the line, like IE and Opera.  Then there's the question of how to do
the clipping.
1. suppress the ellipsis, clip content at the block edge
2. show the ellipsis and clip it at the block edge
3. show the ellipsis if it fits, clip content at the ellipsis edge
(Opera and IE does 2.)
Comment 40 Tantek Çelik 2011-12-14 21:42:00 PST
(In reply to Mats Palmgren [:mats] from comment #39)
> I don't think clipping atomic inlines is the best option.
> Removing atomic inlines occurs in Fx, IE and Opera - only webkit doesn't.
> The parity in Fx is that we do it also for the last character/atomic on
> the line.
> 
> So my suggestion is that we always leave at least one character/atomic
> on the line, like IE and Opera.

Mats, could you attach the reduced test cases that you used to come to these conclusions?

At this point we need reproducible examples for refining the definition for reasons of back compat, which would supercede the conceptually reasoned proposal that Boris made.


> Then there's the question of how to do
> the clipping.
> 1. suppress the ellipsis, clip content at the block edge
> 2. show the ellipsis and clip it at the block edge
> 3. show the ellipsis if it fits, clip content at the ellipsis edge
> (Opera and IE does 2.)

I'm not sure I understand how these look in which case(s) - these similarly would benefit from specifying which test case attachment they're referring to so we can better understand what you mean. Thanks.
Comment 41 Mats Palmgren (:mats) 2011-12-14 21:48:50 PST
Created attachment 581871 [details]
Testcase #3
Comment 42 Mats Palmgren (:mats) 2011-12-16 22:36:30 PST
Created attachment 582489 [details] [diff] [review]
part 1, add Marker::mActive bit

Use a bit on each marker to track if it's active (only overflow:clip means inactive for now); check the flag rather setting the clip edge at infinity to disable ellipsing on a side.

This patch doesn't change the text-overflow behavior in any way.
It's preparation for later patches.
Comment 43 Mats Palmgren (:mats) 2011-12-16 22:40:46 PST
Created attachment 582491 [details] [diff] [review]
part 2, add aFoundVisibleTextOrAtomic param

Make the edge analysis report back if text or atomic inline-level content is visible between the marker edges.

This patch doesn't change the text-overflow behavior in any way.
It's preparation for later patches.
Comment 44 Mats Palmgren (:mats) 2011-12-16 22:49:25 PST
Created attachment 582493 [details] [diff] [review]
part 3, suppress a marker if it makes the line empty

This implements alt. 1 in comment 39.

If the first pass reports back that the line has no visible content
then suppress (mActive=false) the left marker and try again, then
the same thing for the right marker.  If it's still empty with no
markers then re-enable markers and do it again without suppressing.

Builds for testing:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mpalmgren@mozilla.com-1247ef151305/
Comment 45 Mats Palmgren (:mats) 2011-12-16 23:08:13 PST
Created attachment 582494 [details] [diff] [review]
part 4, clip or suppress a marker if it makes the line empty

This implements alt. 2 in comment 39.

If a marker clips an atomic then calculate the maximum marker width that
wouldn't clip it.  Accumulate the maximum of those widths over the line
in Marker::mClipWidth.  Clip the marker to that width if there's no
visible content and try again.

I haven't implemented it for text yet, so when there's a single
character left on the line it will still be removed.  (We need to
use the width of the first character as a minimum width.)
I think this can be done in a follow-up bug, I don't think it's a problem
for web-compat.

Builds for testing:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mpalmgren@mozilla.com-54e0ce882a19/
Comment 46 Mats Palmgren (:mats) 2011-12-16 23:13:26 PST
Created attachment 582495 [details] [diff] [review]
all the parts as one patch
Comment 47 Mats Palmgren (:mats) 2011-12-16 23:17:30 PST
(fyi, it passed reftests locally on Linux and WinXP, but I may have to tweak some
test to get it to pass on Try (anti-aliasing and such)).
Comment 48 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-12-17 03:25:11 PST
Option #2 is a bit weird but Opera and IE9 do it, so I guess it's reasonable to make that the desired behavior.
Comment 49 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-12-18 15:24:36 PST
If we want to go with approach #2, shouldn't you mash together the patches?
Comment 50 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-12-18 15:36:17 PST
Created attachment 582713 [details] [diff] [review]
Parts 3 and 4 combined
Comment 51 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-12-18 15:52:46 PST
Comment on attachment 582713 [details] [diff] [review]
Parts 3 and 4 combined

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

::: layout/generic/TextOverflow.cpp
@@ +385,5 @@
> +        if (rightOverlap > 0 && insideRightEdge && !mRight.mClipped) {
> +          nscoord toEdge = NS_MAX(0, mContentArea.XMost() - borderRect.XMost());
> +          mRight.mClipWidth = NS_MAX(mRight.mClipWidth,
> +                                     NS_MIN(mRight.mWidth, toEdge));
> +        }

I find this confusing. It might be clearer to accumulate the minimum right edge and maximum left edge of the atomic elements in the line, but I guess that ends up a little more complicated.

Also, is it possible to set mLeft.mClipWidth based on one atomic and mRight.mClipWidth based on another atomic and end up with some confusion? It's difficult to reason about.

Seems to me the logic would be clearer if we actually identified the single atomic element's frame that we're going to show on the line.
Comment 52 Mats Palmgren (:mats) 2011-12-19 02:30:06 PST
Created attachment 582773 [details] [diff] [review]
part 3, clip or suppress a marker if it makes the line empty

> It might be clearer to accumulate the minimum right
> edge and maximum left edge of the atomic elements in the line

Ok, I tried to simplify the code.  I'm now accumulating the
innermost edge for frames that are clipped by a marker (left and
right side separately) and then use these result to clip the markers
for the next pass.

> Also, is it possible to set mLeft.mClipWidth based on one atomic and
> mRight.mClipWidth based on another atomic

Yes, both the left and right markers can be clipped, and possibly
by different frames.

I also changed the logic slightly when the last visible character
doesn't fit next to the marker; the previous patch kept the current
behavior that the last character is removed.  This patch instead
disables the marker at that point, so that the text is visible.
I think this is closer to the desired result we want in the end and
it fixes the single-character case.

Builds for testing:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mpalmgren@mozilla.com-f511afafb4ff/
Comment 53 Mats Palmgren (:mats) 2011-12-19 02:32:05 PST
Created attachment 582774 [details] [diff] [review]
all the parts as one patch (wdiff)
Comment 54 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-12-19 02:40:35 PST
Comment on attachment 582773 [details] [diff] [review]
part 3, clip or suppress a marker if it makes the line empty

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

::: layout/generic/TextOverflow.h
@@ +116,5 @@
> +        mAssignedRight = true;
> +      }
> +    }
> +    nscoord left;
> +    nscoord right;

mLeft, mRight
Comment 57 Scoobidiver (away) 2011-12-21 05:19:53 PST
It missed the Firefox 11 release for a few hours.
Comment 58 Mats Palmgren (:mats) 2011-12-21 05:36:35 PST
Comment on attachment 582773 [details] [diff] [review]
part 3, clip or suppress a marker if it makes the line empty

The intention was to take it in Fx 11 (at least).
Comment 59 Scoobidiver (away) 2011-12-21 05:44:55 PST
May be I am wrong about the Firefox 11 target because this changeset is between:
* The latest changeset in 11.0a1/20111220: http://hg.mozilla.org/mozilla-central/rev/2afd7ae68e8b.
* The first changeset in 11.0a2/20111220: http://hg.mozilla.org/mozilla-central/rev/a8506ab2c654
Comment 60 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-12-21 06:06:58 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/32682110d4bf gives an error, showing there's no changeset with that hash in the repository.  Once the aurora uplift makes it to the mozilla-aurora repository (which it hasn't yet), you'll be able to load that link and see whether it made it.  There may be email from Christian Legnitto saying which changeset he was planning to take.
Comment 61 Scoobidiver (away) 2011-12-21 09:36:11 PST
I was wrong about the missed Firefox 11 target.
Comment 62 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-12-21 14:34:30 PST
We might possibly want to take this on beta as well, since it should fix some currently regressed sites.
Comment 63 christian 2011-12-21 15:47:50 PST
Comment on attachment 582773 [details] [diff] [review]
part 3, clip or suppress a marker if it makes the line empty

[triage comment]
Clearing the request for Aurora as it made it in the migration. Please nominate for beta if you feel it is appropriate.
Comment 64 Tantek Çelik 2011-12-21 17:57:49 PST
Created attachment 583694 [details]
simple text-overflow:ellipsis tests for characters, images, inline blocks

I've constructed a text-overflow ellipsis test page that tests one to multiple characters, inline replaced elements, and inline block elements within containing block elements of various sizes.

There are some interesting inconsistencies among Opera 11.6, Chrome 16.0.912.63,  Safari 5.1.1, and Firefox 8. Uploading screenshot next.
Comment 65 Tantek Çelik 2011-12-21 18:14:36 PST
Created attachment 583699 [details]
text-overflow:ellipsis test screenshot in Opera11, Chrome16, Safari5, Firefox8

In particular take a look at how Opera's behavior (leftmost window in screenshot) is inconsistent even just with characters, but especially with mixing characters and inline atomic elements - note that Opera oddly hides an *earlier* character on a line, while showing (clipped) a subsequent image on the same line (3rd and 7th boxes under "with a preceding character").

This leads me to believe that Opera hiding a single character (and not rendering an ellipsis) in the simple text cases is more a bug than intentional behavior. 

The Webkit behavior seems more sensible.

Is IE behaving oddly like Opera or does it have a more consistent behavior?

Mats, please take a look at https://bug690187.bugzilla.mozilla.org/attachment.cgi?id=583694 and see how it behaves with your patches in comparison.
Comment 66 Mats Palmgren (:mats) 2011-12-22 02:29:43 PST
> This leads me to believe that Opera hiding a single character (and not
> rendering an ellipsis) in the simple text cases is more a bug than
> intentional behavior. 

I don't know...  you have to ask them.

> The Webkit behavior seems more sensible.

In the last character case?  Yes, having an ellipsis is
certainly more sensible than an empty line.

> Is IE behaving oddly like Opera or does it have a more consistent behavior?

It looks like you're testing an old version of IE?
My IE10 on Windows 8 doesn't look like your screenshot -
it's clipping the marker when it doesn't fit next the
last item on the line, in both the atomic and text case.
Which is exactly how we intend to do it.  Firefox 11 is
very close, but there's a bug in the text case as noted
in comment 52.  I'll fix that for Fx12 in bug 712894.

For comparing, you can try http://nightly.mozilla.org/
Comment 67 Tantek Çelik 2011-12-22 12:24:10 PST
(In reply to Mats Palmgren [:mats] from comment #66)
> > Is IE behaving oddly like Opera or does it have a more consistent behavior?
> 
> It looks like you're testing an old version of IE?
> My IE10 on Windows 8 doesn't look like your screenshot -

I didn't include a screenshot of IE, just Opera, Chrome, Safari, Firefox in that order.

> it's clipping the marker when it doesn't fit next the
> last item on the line, in both the atomic and text case.

I don't think I understand.

Could you post a screenshot of what you're seeing in IE10 of https://bug690187.bugzilla.mozilla.org/attachment.cgi?id=583694 
?



> Which is exactly how we intend to do it.

If you think IE10's behavior is the most reasonable that sounds good but I'd still like to compare. There are strange things in Webkit's behavior like how it ellipses the last character in the text-only case inside the 1em wide container, but then inexplicably switches back to *showing* the last character in the narrower 0.5em container.


> Firefox 11 is
> very close, but there's a bug in the text case as noted
> in comment 52.  I'll fix that for Fx12 in bug 712894.
> 
> For comparing, you can try http://nightly.mozilla.org/

Ok I will try nightly. Thanks.
Comment 68 Mats Palmgren (:mats) 2011-12-22 13:23:12 PST
Created attachment 583917 [details]
IE10 screenshot

> I didn't include a screenshot of IE...

Heh, I thought the one on the right was IE.  :-)

Here's a screenshot of IE10 on Windows 8.
Comment 69 Alex Keybl [:akeybl] 2012-01-05 12:41:07 PST
As mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=690187#c63, please nominate for beta approval if deemed high-value and low-risk. No need to track though.

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