Closed Bug 1215182 Opened 9 years ago Closed 9 years ago

[css-grid] Make our "Implicit Named Areas" handling match the spec

Categories

(Core :: Layout, defect)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla45
Tracking Flags:
Tracking Status
firefox44 --- affected
firefox45 --- fixed

People

(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)

References

Details

Attachments

(2 files)

fix
3.07 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
reftest
13.47 KB, patch
Details | Diff | Splinter Review 
https://lists.w3.org/Archives/Public/www-style/2015Oct/0040.html

It appears there are no conditions whatsoever other than the existence
of at least one line of the form X-start or X-end for the "area" X
to exist.
Attached patch fix — — Splinter Review 

        Attachment #8674344 -
        Flags: review?(dholbert)

    
    

    
  
I'm forgetting why we need to maintain these areas in the first place. Can we do away with it & just deal with lines?

In particular, Tab said the following in the thread that you linked to:
  # The spec doesn't actually "create" any such areas; they exist as a
  # spec concept for convenience only.  The placement properties never
  # care about areas, they just check for lines with particular names.
https://lists.w3.org/Archives/Public/www-style/2015Oct/0052.html

Given this, I'm wondering why we're creating structures for these areas internally. Was it just an optimization?

    
    

    
  
I think the answer is basically "yes, it's an optimization".

IIUC: given a grid item with "grid-row-start:A", the lazy thing to do is to search first for "A-start" in our explicit-line-list [and also look for explicit areas named "A"], and if we fail, then search our explicit-line-list *again* for just "A".  (This is basically what tab described in the post I linked to.)

For authors who just use "A" naming for their lines (who don't name anything "A-start" etc.), the first search is doomed & wasteful. This sucks, because we have to perform it for each grid item that uses a named line.  So, the ImplicitNamedAreas object (a hash set populated during a precomputation step) lets us optimize these doomed searches away.

Does this make sense?

(I slightly wonder whether we should make the further optimization of hashing the actual line-names, so we can optimize away the second search as well, and perhaps end up with better code-sharing/symmetry as a result. For this to help, authors would have to be using completely-undefined line names, though, and maybe that's not a case that's worth optimizing for.)

    
    

    
  
Comment on attachment 8674344 [details] [diff] [review]
fix

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

r=me -- I've just got one comment-nit, below:

::: layout/generic/nsGridContainerFrame.cpp
@@ +1162,5 @@
>  nsGridContainerFrame::AddImplicitNamedAreas(
>    const nsTArray<nsTArray<nsString>>& aLineNameLists)
>  {
>    // http://dev.w3.org/csswg/css-grid/#implicit-named-areas
> +  // A line named 'x-start' or 'x-end' implies an implicit named area 'x'.

To me, this implies that we'll be creating an actual area (with resolveable start/end lines in all dimensions).  But that's not what actually happens.

Might be worth adding a parenthetical clarifying this -- e.g....
  // (But only explicitly-defined edges of this implicit area can be
  // matched against during grid-item placement.)

...or something along those lines. (I think this is accurate? Please correct me if not.)

So for example a row-line named "x-start" will create an implicit named area "x", but it doesn't actually influence how "grid-row-end: x" is resolved, or how "grid-column-start/end" are resolved.  (It means we'll take a slightly different codepath -- we'll traverse the HasImplicitNamedArea()-guarded code -- but we'll fail to match there, I think.)

        Attachment #8674344 -
        Flags: review?(dholbert) → review+

    
    

    
  
Comment on attachment 8674345 [details] [diff] [review]
reftest

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

One drive-by nit on the reftest patch:

::: layout/reftests/css-grid/grid-placement-implicit-named-areas-001.html
@@ +6,5 @@
> +<html><head>
> +  <meta charset="utf-8">
> +  <title>CSS Grid Test: implicit named areas</title>
> +  <link rel="author" title="Mats Palmgren" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1215182">
> +  <link rel="help" href="http://dev.w3.org/csswg/css-grid/#abspos-items">

I think this #abspos-items anchor is incorrect. (probably from copypaste error)

You probably really want to link to http://dev.w3.org/csswg/css-grid/#grid-placement-slot

(This applies to the testcase as well as the reference case)

    
    

    
  
(In reply to Daniel Holbert [:dholbert] from comment #4)
> I think the answer is basically "yes, it's an optimization".

Yes, it's just an optimization now.

    
    

    
  
(In reply to Daniel Holbert [:dholbert] from comment #5)
> >    // http://dev.w3.org/csswg/css-grid/#implicit-named-areas
> > +  // A line named 'x-start' or 'x-end' implies an implicit named area 'x'.
> 
> To me, this implies that we'll be creating an actual area (with resolveable
> start/end lines in all dimensions).  But that's not what actually happens.

I removed that line.  The spec link is sufficient.

(I think you're comment here illustrates perfectly why "implicit named area"
is not only a useless, but also harmful, concept.)


(In reply to Daniel Holbert [:dholbert] from comment #6)
> You probably really want to link to
> http://dev.w3.org/csswg/css-grid/#grid-placement-slot

Fixed, thanks.

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/7b8f601030db
https://hg.mozilla.org/mozilla-central/rev/dc90c209ea61
Status: NEW → RESOLVED
Closed: 9 years ago

          status-firefox45:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: