Closed Bug 1456590 Opened 2 years ago Closed 2 years ago

getGridFragments provides line names from implicit named areas, and it's confusing

Categories

(DevTools :: Inspector, defect, P2)

61 Branch
defect

Tracking

(firefox61 fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: ewright, Assigned: bradwerth)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Sometimes, in css grid autocomplete, the grid names supplied are incorrect.

STR:
- open devtools inspector
- go to https://gridbyexample.com/examples/code/example7.html, or a page with grid names.
- select a box
- in the rules editor type "grid-row", hit enter
- observe the autocomplete popup
- add multiple rules for "grid-row", "grid-area" and "grid-column" each time look at the autocomplete popup.
- After adding a few rules (3-4) the names start to repeat themselves and/or row names are suggested for column rules and vice versa.

ER:
- Only row names are suggested for `grid-row` and only column names for `grid-column`, both are displayed for `grid-area`. Each name is only displayed once.

I added a console.log in `server/actors/layout.js`, line 104, you can see that the data coming from `getGridFragments();` is incorrect.

I believe the problem comes directly from `getGridFragments` in `dom/base/Element.cpp`.

Screenshots are attached to illustrate.
Flags: needinfo?(bwerth)
Blocks: dt-grid
Priority: -- → P2
Assignee: nobody → bwerth
Flags: needinfo?(bwerth)
Slightly simpler steps to reproduce:

- open devtools inspector
- go to https://gridbyexample.com/examples/code/example7.html, or a page with grid names.
- select box C
- in the rules editor for class .c type "grid-row: row2-end;", hit enter
- add another rule below that with "grid-row:" and then observe the autocomplete popup. Amongst the other names, it will contain the names "col1-start", "col2-start", and "col3-start".
I've come to the conclusion that the behavior is correct, and matches what is demanded by the specification. This will take some explaining...

The example has a line named "col1-start" in the grid-template-columns declaration. Having a line name with "-start" or "-end" defines a grid area, per spec:

https://www.w3.org/TR/css-grid-1/#implicit-named-areas

You can see that this is so in the screenshot, where getGridFragments() is reporting several named areas, among them "col1". So what are the bounds of the "col1" area? The spec is not definite on this, but in our implementation, we act as if this area was explicitly named and we follow:

https://www.w3.org/TR/css-grid-1/#implicit-named-lines

Taken together, this means that a single "col1-start" line in the grid-template-columns declaration creates a "col1" area, which creates an implicit "col1-end" line in the columns AND IT CREATES TWO ROW LINES "col1-start" and "col1-end". When our API reports line names, we supply the implicit line names of any areas that could apply. So when box C in the example has been moved to "row2-end" and has created a new implicit row line, the "col1-start" name is assigned to it. "col1-end" is not assigned, because the area must have at least one cell in it, so "col1-end" would be the name applied to the next implicit line when and if it is created.

You can demonstrate this by changing the example to use just "col1" instead of "col1-start" for the line name. In that case, there are no "col1" lines in the grid-row autocomplete popup.

All that said, it's understandable that this behavior is not a great UI experience. If a designer was not intending to create an implicit named area, and just happened to use a line name with "-start" or "-end", then this will be confusing.

I'll change the getGridFragments() API to omit line names from implicit grid areas.
Summary: getGridFragments sometimes return incorrect information → getGridFragments provides line names from implicit named areas, and it's confusing
I have no opinion on whether we should show these line names or not
in the devtools auto-completion UI (but they definitely do exist
as you explain comment 2).  Does devtools people / Jen Simmons have
an opinion on this?

In case we do want to suppress them, the question is then where...
Your patch does it at the Grid API level, making it impossible
to know about these names at all, IIUC.  Another way of fixing it
would be to report them separately as "implicit line names" and
let the API consumer decide what to do with them.  We might want
to exclude them from auto-completion, but include them in other UI...

IIRC, there are three sources of line names:
1. explicit line names (from grid-template-columns/rows)
2. implicit line names from explicit grid areas, e.g.
     grid-template-areas: "a"
   creates implicit "a-start" "a-end" line names in both axes.
3. implicit line names from implicit areas (which is what this
   report is about), i.e. any use of a line name ending in -start or
   -end (e.g. x-end) automatically also creates an implicit area
   named x which then has four implicit lines (x-start and x-end in
   each axis) associated with it (same as for explicit areas)

Perhaps we should report these line names separately and leave it
to the caller to decide how to use them?

BTW, the auto-completion UI currently only lists the *line* names
(a-start/a-end), but using the *area* name directly is also a valid
option ('grid-column:a' is short for 'grid-column: a-start/a-end').
Maybe that's intentional though since there are some restrictions
on matching there, e.g. 'grid-column:a 1' is nonsensical if 'a' is
an area name only.  Perhaps we should include explicit area names
though to make the author aware of that option?
I really like Mats' proposal to keep all of the names but have getGridFragments return them in a way that makes the UI able to know the nature for each of them.

Additionally, a simple fix on the UI side is to de-dupe the list.
(I filed bug 1458893 about including area names in the auto-completion,
to avoid cluttering this bug with separate issues.  They are somewhat
related though, since we should probably exclude implicit area names
too if we exclude implicit line names here.)
Status: NEW → ASSIGNED
(In reply to Patrick Brosset <:pbro> from comment #7)
> I really like Mats' proposal to keep all of the names but have
> getGridFragments return them in a way that makes the UI able to know the
> nature for each of them.

If that's what's needed, devtools already has the information needed. Every grid object (area, line, track) has a "type" property which is either "explicit" or "implicit". In this case, the desired behavior could be achieved in devtools by excluding lines where all of the below are true:

1) The line name is "-start" or "-end".
2) The line is type implicit.
3) The area with the same name as the first part of the line name (before -start or -end) is type implicit.
(In reply to Brad Werth [:bradwerth] from comment #9)
> (In reply to Patrick Brosset <:pbro> from comment #7)
> > I really like Mats' proposal to keep all of the names but have
> > getGridFragments return them in a way that makes the UI able to know the
> > nature for each of them.

I'll offer my preference here. Since devtools is the only consumer of the Chrome-only getGridFragments API, it makes sense to me to tailor its output to what devtools actually wants to see and use. This will also help keep different parts of devtools in agreement. For example, if the API never reports implicit line names from implicit areas, then when we display the line names on each line, we don't need to do this filtering again to ensure we match the autocomplete popup. This is what the proposed patch does -- it keeps the filtering in the getGridFragments API.

I also want to point out that there are already implicit line names that we don't include in the getGridFragments API. Consider:

grid-template-columns: [first-explicit-name] 10px [last-explicit-name];
grid-column: last-explicit-name / 7 some-implicit-name

This will create 7 implicit lines, all of which have some-implicit-name for purposes of layout. But getGridFragments doesn't report those names. The change requested in this bug feels consistent with that.
> This will create 7 implicit lines, all of which have some-implicit-name
> for purposes of layout

True, lines in the implicit grid match *any* name, but they aren't
really "named".  The ones we're discussing are implicit *named* lines
in spec parlour.  There might be a desire to present them differently
in author-oriented tools.
Comment on attachment 8972734 [details]
Bug 1456590 Part 1: Omit line names from implicitly named areas, to avoid confusion.

https://reviewboard.mozilla.org/r/241264/#review247236

I think this is fine for now.  We can add them separately later
if there's a desire to present them in devtools UI in some way.
Attachment #8972734 - Flags: review?(mats) → review+
Comment on attachment 8972735 [details]
Bug 1456590 Part 2: Update test expectations to check that names from implicit named areas are not assigned to lines.

https://reviewboard.mozilla.org/r/241266/#review247378
Attachment #8972735 - Flags: review?(mats) → review+
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4701762470f2
Part 1: Omit line names from implicitly named areas, to avoid confusion. r=mats
https://hg.mozilla.org/integration/autoland/rev/f872dc290835
Part 2: Update test expectations to check that names from implicit named areas are not assigned to lines. r=mats
https://hg.mozilla.org/mozilla-central/rev/4701762470f2
https://hg.mozilla.org/mozilla-central/rev/f872dc290835
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Product: Firefox → DevTools
See Also: → 1526975
You need to log in before you can comment on or make changes to this bug.