Last Comment Bug 669284 - Checkbox styled with margin:0 is replaced by ellipsis when next to the edge in a text-overflow block
: Checkbox styled with margin:0 is replaced by ellipsis when next to the edge i...
Status: VERIFIED FIXED
[inbound] text-overflow regression in 7
: regression, testcase
Product: Core
Classification: Components
Component: Layout (show other bugs)
: 7 Branch
: All All
: -- minor with 1 vote (vote)
: mozilla8
Assigned To: David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
:
Mentors:
http://jsfiddle.net/yanndinendal/qKAQ...
Depends on:
Blocks: 312156
  Show dependency treegraph
 
Reported: 2011-07-05 05:23 PDT by Yann Brelière
Modified: 2013-12-27 14:27 PST (History)
9 users (show)
mats: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
Testcase #1 (485 bytes, text/html)
2011-07-05 07:30 PDT, Mats Palmgren (:mats)
no flags Details
hack (842 bytes, patch)
2011-07-05 09:02 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
Testcase #2 (1.03 KB, text/html)
2011-07-05 10:14 PDT, Mats Palmgren (:mats)
no flags Details
patch to apply native theme overflow only to visual overflow and not scrollable (2.37 KB, patch)
2011-07-05 11:29 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
bzbarsky: review+
asa: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
fix + test (6.38 KB, patch)
2011-07-05 11:38 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review

Description Yann Brelière 2011-07-05 05:23:54 PDT
User Agent: Mozilla/5.0 (X11; Linux i686; rv:7.0a1) Gecko/20110704 Firefox/7.0a1
Build ID: 20110704210442

Steps to reproduce:

I have put both a checkbox input and some text in a parent having `text-overflow: ellipsis;`.


Actual results:

Whether or not the text is too long (in which case it will have a proper ellipsis at the end), the checkbox will always be replaced by an ellipsis!

You can see the html/css and the result in this test case:
http://jsfiddle.net/yanndinendal/qKAQc/


Expected results:

In Chrome, the checkbox is not replaced by an ellipsis. Only the text has an ellipsis when it is too long.
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2011-07-05 05:37:12 PDT
Yeah, this is broken.  roc?

We need to track this for 7; this is a serious regression, imo.
Comment 2 Mats Palmgren (:mats) 2011-07-05 07:30:45 PDT
Created attachment 543930 [details]
Testcase #1

The problem is caused by the following style rule from
http://fiddle.jshell.net/css/normalize.css

input {
	margin:0;
}
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2011-07-05 07:40:26 PDT
Hmm.  Is that making the checkbox overflow to the left or something?
Comment 4 Mats Palmgren (:mats) 2011-07-05 07:43:39 PDT
Yes, it appears our implementation of <input type="checkbox"> is broken in
the sense that it causes overflow when styled with margin:0.
The text-overflow implementation does the right thing in this case
and removes the [atomic inline-level] element.
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2011-07-05 07:46:35 PDT
> It causes overflow when styled with margin:0

Why, exactly?  What's overflowing?

If you set a negative margin, does the checkbox disappear in other UAs too?
Comment 6 Mats Palmgren (:mats) 2011-07-05 09:02:13 PDT
Created attachment 543971 [details] [diff] [review]
hack

The overflow comes from nsNativeThemeGTK::GetWidgetOverflow.
The bug does not occur when I nuke it with this patch.

Hmm, I suppose we really do want theme shadows and such to actually overflow...

So, I guess we could make text-overflow take such overflow into account
(and ignore it).  Are there other (simpler) solutions or workarounds?
Comment 7 Mats Palmgren (:mats) 2011-07-05 10:14:15 PDT
Created attachment 543987 [details]
Testcase #2
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2011-07-05 11:12:41 PDT
Hmm.  Are those layout overflow or painting overflow?  I'd think that text/overflow should only consider the former, not the latter....
Comment 9 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-07-05 11:23:46 PDT
Currently reported widget overflow affects both scrollable and visual overflow (see nsIFrame::FinishAndStoreOverflow in layout/generic/nsFrame.cpp).  I agree that it does make sense to change that -- I suppose if there are widgets for which such a change doesn't make sense, they're not reporting their size correctly.

(I agree that considering the interaction of text-overflow with overflow, and that overflow operates on scrollable overflow, I think text-overflow should operate on scrollable overflow.)
Comment 10 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-07-05 11:29:47 PDT
Created attachment 544005 [details] [diff] [review]
patch to apply native theme overflow only to visual overflow and not scrollable

I suspect this will fix it, but I don't see the issue with my GTK theme.  Could somebody who does test this?
Comment 11 Mats Palmgren (:mats) 2011-07-05 11:38:40 PDT
Created attachment 544008 [details] [diff] [review]
fix + test

Here's a somewhat less risky fix if we need it...
It ignores overflow if it's within the overflow created by the widget theme.
Comment 12 Mats Palmgren (:mats) 2011-07-05 11:49:54 PDT
(In reply to comment #10)
> Could somebody who does test this?

Yes, it fixes the problem.
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2011-07-05 11:57:40 PDT
dbaron's patch fixes this for me on Mac too.
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2011-07-05 11:58:28 PDT
Comment on attachment 544005 [details] [diff] [review]
patch to apply native theme overflow only to visual overflow and not scrollable

r=me
Comment 15 Mats Palmgren (:mats) 2011-07-05 12:54:36 PDT
Fwiw, I tested dbaron's patch with various themes on Linux and I couldn't
find any errors anywhere in the UI.  I pushed it together with my reftest
to Try:  http://tbpl.mozilla.org/?tree=Try&rev=1ec682161801
Comment 17 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-07-06 11:12:57 PDT
Thanks for landing that.
Comment 18 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-07-06 11:16:26 PDT
Comment on attachment 544005 [details] [diff] [review]
patch to apply native theme overflow only to visual overflow and not scrollable

Requesting approval for mozilla-aurora since this has become a bit of a problem for text-overflow.

This changes the way we handle native-themed widgets (generally form controls) that report overflow beyond their bounds.  We used to allow scrolling to such overflow; but this changes things so such overflow is not considered scrollable overflow (but is still considered visual overflow so it will repaint correctly).

It's not without risk, since it does intentionally change Web-related behavior, but in general it should only change the scrollable extents by a small number of pixels, and I think it's the right thing to do and I'm comfortable making this change early in the aurora cycle.
Comment 21 Virgil Dicu [:virgil] [QA] 2011-08-19 02:38:52 PDT
Mozilla/5.0 (X11; Linux x86_64; rv:7.0) Gecko/20100101 Firefox/7.0

Verified this issue on F7beta1 and latest Nightly using test case 2 and an ellipsis still appears next to number 4 (all other check-boxes are displayed correctly). On Chrome, all check-boxes appear correctly. 

Checked on Windows XP, Windows 7, Ubuntu 11.04 and Mac OS X 10.6.

Should this issue be re-opened?
Comment 22 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-08-19 07:27:53 PDT
The test next to number 4 has a negative margin, so I'd expect it to have overflow.
Comment 23 Virgil Dicu [:virgil] [QA] 2011-08-25 05:15:11 PDT
Setting status to Verified Fixed based on comment 22.

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