Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add or update dfns for definitions #425

Merged
merged 1 commit into from
May 1, 2018
Merged

Add or update dfns for definitions #425

merged 1 commit into from
May 1, 2018

Conversation

dstorey
Copy link
Member

@dstorey dstorey commented Apr 27, 2018

Fixes #417

I've only touched the main spec and not the various sub specs, as that will take a while.

I included data-dfn-exports (seems to be a typo of using dfn-exports in the spec) to most things. I don't know if this is correct, but it seems if we want to mark things up as definitions then they are thinks that should be useful to export/link to.

dstorey
dstorey commented Apr 27, 2018
View reviewed changes
master/struct.html
@@ -2302,7 +2301,7 @@ <h3 id="roleattribute">Role attribute</h3>
<th>Animatable</th>
</tr>
<tr>
<td><dfn id="RoleAttribute">role</dfn></td>
<td><dfn id="RoleAttribute" data-dfn-type="element-attr" data-dfn-export>role</dfn></td>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs a data-dfn-for, but I've no idea how to say apply to all elements without listing every single SVG element.

dstorey
dstorey commented Apr 27, 2018
View reviewed changes
master/types.html
@@ -1271,7 +1271,7 @@ <h3 id="ListInterfaces">List interfaces</h3>
</li>
</ol>

<p>Whenever a list element object is to be <dfn id="TermDetach">detached</dfn>,
<p>Whenever a list element object is to be <dfn id="TermDetach" data-dfn-type="dfn" data-dfn-export>detached</dfn>,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some like these that are very generic words. Not sure if they should be exported or have more descriptive labels applied.

dstorey
dstorey commented Apr 27, 2018
View reviewed changes
master/types.html
@@ -1578,7 +1578,7 @@ <h2 id="DOMInterfacesForReflectingSVGAttributes">DOM interfaces for reflecting S

</div>

<p>Some IDL attributes <dfn id="TermReflect">reflect</dfn> the value of a content attribute
<p>Some IDL attributes <dfn id="TermReflect" data-dfn-type="dfn">reflect</dfn> the value of a content attribute
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figured reflect must be cononically defined in some other spec as it is a common DOM/HTML term. so didn't add the data-dfn-exports. Should these type of terms be marked up as dfn or just linked to whichever of DOM or HTML that defines it? Same applies to a few other things in SVG like stacking contexts etc.

@dstorey
Copy link
Member Author

dstorey commented Apr 27, 2018

This probably needs experts from each section to check to make sure I marked things up correctly.

dirkschulze
dirkschulze reviewed Apr 27, 2018
View reviewed changes
Copy link
Contributor

@dirkschulze dirkschulze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern is that you removed the global definitions that are supposed to affect the children automatically. This is error prone for future additions in this list.

Otherwise looks good to me.

master/types.html
@@ -1623,7 +1623,7 @@ <h2 id="DOMInterfacesForReflectingSVGAttributes">DOM interfaces for reflecting S
</ol>

<p>When a <a>reflected</a> content attribute is to be
<dfn id="TermReserialize">reserialized</dfn>, optionally using a
<dfn id="TermReserialize" data-dfn-type="dfn" data-dfn-export>reserialized</dfn>, optionally using a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that a term we want to get referenced from outside? Seems very generic. But of course, another spec could always clarify which spec it wants to link to in case the term exists already.

master/coords.html
an element's geometric shape and its <a>stroke shape</a>.</li>

<li>The <dfn id="TermDecoratedBoundingBox" data-dfn-type="dfn" data-export="">decorated bounding box</dfn> is the bounding box that contains
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was the data-export set manually? On that note... our XML based tools are not affected by attributes w/o values?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure but it was in a bunch of places. I'm not familiar with the XML tools, but there was a definitions class all over the source in single quotes. Wouldn't that break the XML tools too?

master/interact.html
@@ -358,9 +358,9 @@ <h2 id="PointerEvents">Pointer events</h2>

<h2 id="pointer-processing">Hit-testing and processing order for user interface events</h2>

<dl class='definitions' data-dfn-type="dfn" data-export="">
<dl class="definitions">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, data-dfn-type specified on a dl marks child dts to the same type. You could check the spec.json that bikeshed downloads and see if those items show up as dfn.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wasn't the original issue that these DLs didn't have dfn? The one called out was the DL on https://svgwg.org/svg2-draft/render.html#Definitions which has this pattern. But maybe it was due to using data-exports rather than data-dfn-exports?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe. Don't know.

dirkschulze
dirkschulze approved these changes May 1, 2018
View reviewed changes
Copy link
Contributor

@dirkschulze dirkschulze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let’s get this in. We can move some definitions per group later on as well. The current change isn’t wron just more verbose in some cases.

@dstorey dstorey merged commit 25f5a49 into w3c:master May 1, 2018
@dirkschulze
Copy link
Contributor

I start seeing reference issues on CSSTransforms now: https://drafts.csswg.org/bikeshed/css-transforms-1/

Those likely are a result of this PR.

For example, "presentation attribute":

No 'dfn' refs found for 'presentation attributes' that are marked for export.

cd00984#diff-7165aa55468cbc8593fcbb8e9b5cb2ecR307

@plinss Is there a mistake that we did here? It seems like the dfn is marked for export.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dirkschulze dirkschulze dirkschulze approved these changes

@boggydigital boggydigital Awaiting requested review from boggydigital

@AmeliaBR AmeliaBR Awaiting requested review from AmeliaBR

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dstorey @dirkschulze