Closed Bug 1019470 Opened 10 years ago Closed 10 years ago

CSS - LI default left padding after bullet disappeared

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Version:
32 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla32
Tracking Flags:
Tracking Status
firefox30 --- unaffected
firefox31 --- unaffected
firefox32 + fixed
firefox33 --- verified

People

(Reporter: lh, Assigned: jfkthame)

References

(
URL
)

Details

(4 keywords)

Attachments

(2 files, 1 obsolete file)

restore space after list-style-image bullet.
(deleted), patch
MatsPalmgren_bugz
: review+
xidorn
: feedback+
Details | Diff | Splinter Review
reftest for list-style-image spacing.
(deleted), patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0 (Beta/Release)
Build ID: 20140602030202

Steps to reproduce:

<!DOCTYPE html>
<html>
<head>
<style>
LI {
    list-style-image: url('http://cdn2.artwhere.net/www.cinetelerevue.be/design/bleu.gif');
}
</style>
</head>
<body>
<ul>
    <li>test</li>
</ul>
</body>
</html>


Actual results:

http://screencast.com/t/fQClyQuHF9f


Expected results:

http://screencast.com/t/yIWIsAyP1

This result comes from Firefox 29, Chrome, ...
Keywords: css2
Summary: LI default left padding after bullet disappeared → CSS - LI default left padding after bullet disappeared
I can reproduce this on mac as well.
URL: data:text/html;charset=utf-8,<!DOCTYP...
Status: UNCONFIRMED → NEW
Component: Untriaged → General
Ever confirmed: true
Keywords: regression, regressionwindow-wanted, reproducible, testcase
OS: Windows 7 → All
Product: Firefox → Core
Hardware: x86_64 → All
Jonathan, this looks like bug 1013160 - is this change intentional? That would surprise me, but maybe I'm missing something...
Blocks: 1013160
Component: General → Layout
Flags: needinfo?(jfkthame)
Keywords: regressionwindow-wanted
This may need to be discussed in CSSWG as they have decided that the spacing only depends on the suffix. So many influences by that spec...
I'd like to add a half em spacing to image bullets like what we have done for graphic bullets.
Version: 32 Branch → Trunk
Something like this should be OK; we may want to raise it in the WG for further discussion, but for now I think we need a simple fix for the visible regression.
Attachment #8433275 - Flags: review?(matspal)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Flags: needinfo?(jfkthame)
Version: Trunk → 32 Branch
Comment on attachment 8433275 [details] [diff] [review]
restore space after list-style-image bullet.

Xidorn, is this the sort of thing you also had in mind here? (Your comments came in while I was rebuilding, so I saw them after writing the patch!)
Attachment #8433275 - Flags: feedback?(quanxunzhen)
Comment on attachment 8433275 [details] [diff] [review]
restore space after list-style-image bullet.

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

(In reply to Jonathan Kew (:jfkthame) from comment #7)
> Comment on attachment 8433275 [details] [diff] [review]
> restore space after list-style-image bullet.
> 
> Xidorn, is this the sort of thing you also had in mind here? (Your comments
> came in while I was rebuilding, so I saw them after writing the patch!)

Yes, this patch LGTM.

I just wonder if there is any helper function which makes it convenient to append space to a nsMargin according to the writing mode. I don't really like the duplication here. Anyway, as a temporary fix, it should be good.
Attachment #8433275 - Flags: feedback?(quanxunzhen) → feedback+
(In reply to Xidorn Quan from comment #8)
> I just wonder if there is any helper function which makes it convenient to
> append space to a nsMargin according to the writing mode.

Not at the moment, AFAIK. Maybe we will eventually switch over the mPadding field to be a LogicalMargin, as part of doing more of the work in logical coordinates instead of physical, in which case it'd become trivial.
Comment on attachment 8433275 [details] [diff] [review]
restore space after list-style-image bullet.

>+      // Add spacing to the padding

Nit: please add a full stop to end the sentence.
(also in the other place)

Will you add a reftest for this later?
(perhaps we could extend bullet-space-1.html for this case?)
Attachment #8433275 - Flags: review?(matspal) → review+
I'm curious why we treat both vertical directions the same.
I'd naively think that the bullet padding in bottom-to-top writing mode
would be on the top side. (Or do we not support bottom-to-top?)
Flags: in-testsuite?
(In reply to Mats Palmgren (:mats) from comment #11)
> I'm curious why we treat both vertical directions the same.
> I'd naively think that the bullet padding in bottom-to-top writing mode
> would be on the top side. (Or do we not support bottom-to-top?)

Right, it's not entirely clear how far we should/will support bottom-to-top. The two important vertical cases are both top-to-bottom, with the block direction being either left-to-right or right-to-left.

The additional case would be wm.IsVertical() && !wm.IsBidiLTR(), but I think it may be premature to add code for that at this stage, until we're closer to actually having usable vertical-text support.

(In reply to Mats Palmgren (:mats) from comment #10)
> Will you add a reftest for this later?
> (perhaps we could extend bullet-space-1.html for this case?)

Yes, I was thinking it should be possible to do a similar test here.
Attached patch reftest for list-style-image spacing. (obsolete) (deleted) — — Splinter Review 

        Attachment #8433557 -
        Flags: review?(matspal)

        Attachment #8433557 -
        Flags: review?(matspal) → review+

    
    

    
  


  
Actually, I think we can do the comparison more simply and directly in this case. (Sorry about the churn.)

        Attachment #8433570 -
        Flags: review?(matspal)

    
  

        Attachment #8433557 -
        Attachment is obsolete: true

        Attachment #8433570 -
        Flags: review?(matspal) → review+
Blocks: 1021238
No longer blocks: 1021238
QA Whiteboard: [good first verify]

    
    

    
  
bug resolved for Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:33.0) Gecko/20100101 Firefox/33.0 buildID	20140716030202

http://imgur.com/oLTSd8w,D10ucap

[bugday-20140716]
QA Whiteboard: [good first verify] → [good first verify] [bugday-20140716]

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: