Append a self-link to each heading #30

annevk · 2016-10-24T14:34:20Z

domenic

This isn't quite right, although I'm excited you started on it.

All the TOC entries have a section symbol in them now.

The DOM structure here:

<h4 id="dom-trees">2.1.3 DOM trees <a href="#dom-trees">§</a></h4>

is not equivalent to the DOM structure used in our Bikeshed-based specs:

<h3 class="heading settled" data-level="3.5" id="defining-event-interfaces"><span class="secno">3.5. </span><span class="content">Defining event interfaces</span><a class="self-link" href="#defining-event-interfaces"></a></h3>

which will make styling difficult.

domenic · 2016-10-24T19:04:31Z

I'm going to try to push a series of commits that get our DOM structure closer to the Bikeshed one. We'll need to update the standard.css file as well I think.

in <span class="secno">

Bikeshed doesn't include it in the TOC, but does in the document, so don't even bother trying to be consistent with Bikeshed, since it's inconsistent with itself.

domenic · 2016-10-24T19:23:10Z

With the latest commits we have

<h4 id="dom-trees" class="heading"><span class="secno">2.1.3 </span>DOM trees<a href="#dom-trees" class="self-link"></a></h4>

which is pretty close to Bikeshed. I didn't create the <span class="content"> wrapper since it seems unused.

We'd merge this and then transfer some stuff from https://resources.whatwg.org/bikeshed.css to https://resources.whatwg.org/standard.css. I've tested that this works locally. \o/

domenic · 2016-10-24T19:23:39Z

The secno fix also helps with #27

annevk · 2016-10-25T06:40:14Z

It seems you now overwrite class="no-num". Having .heading also seems somewhat redundant, but maybe I'm missing something.

domenic · 2016-10-25T13:59:14Z

Ah dang yeah no-num is probably broken.

The class=heading is used by a bunch of the styles in our two stylesheets, but technically we could replace it with a bunch of selectors targeting the individual tag names.

annevk · 2016-10-25T14:07:15Z

Okay, I guess we can keep .heading for now until someone wants to clean that up... So I guess instead we should get the current attribute value and then append " heading" to it and then set it again?

domenic · 2016-10-25T14:16:24Z

I guess we should poke around Wattsi to see if there's some kind of classList abstraction in use, but otherwise that sounds reasonable... Hmm or we could find the place that currently sets it to "no-num" and change it to set to either "heading" or "heading no-num".

annevk · 2016-10-25T14:19:44Z

As far as I can tell Wattsi just uses literal comparison on classes. Which is also why changing them can be potentially problematic since things like class <> 'no-num' would start yielding true for heading no-num...

annevk · 2016-10-25T14:34:58Z

It doesn't seem like there's too much class manipulation (in wattsi.pas) so whatever way we go seems doable. Still a little hesitant to add heading (when does it apply?) though. Rather not add redundant markup as we already have so much markup.

domenic · 2016-10-25T14:45:30Z

Sure, we don't have to add heading; we just have to be careful to update all the CSS.

Do you want to take this over now perchance? :)

annevk · 2016-10-25T15:22:42Z

It seems the equivalent of .heading is :has(h2,h3,h4,h5,h6), but we'll have to write the latter out as :has() is not supported yet.

See whatwg/wattsi#30 for context.

annevk · 2016-10-26T08:30:48Z

I don't really understand why you added secno either. It doesn't seem to be needed. Neither of our stylesheets use it so I'm going to undo that change as well.

annevk · 2016-10-26T08:38:08Z

src/wattsi.pas

+                  Scratch.Append('#');
+                  ExtractedData := Element.GetAttribute('id');
+                  Assert(not ExtractedData.IsEmpty);
+                  Scratch.AppendDestructively(ExtractedData); // appending LastSeenHeadingID does not work


@Hixie could you explain why simply appending LastSeenHeadingID does not work here? I don't understand how it can be different from GetAttribute('id'). Thanks!

What does it do instead?

Oh, I see. It's because you're using AppendDestructively, which takes a Rope or CutRope, not a string.

The reason you can't use LastSeenHeadingID is that it's a local variable, so if you appended it, you would end up having the DOM tree contain a pointer to the stack (rather than the heap). The solution is to not use AsString on the output of EnsureID. Instead, get back a CutRope, which contains pointers to the heap.

Note that EnsureID is itself just going GetAttribute, so it wouldn't make much difference in practice. Also be aware that you can only use AppendDestructively once per reference (hence the name), so if you use LastSeenHeadingID again you'd have to get it back out of the DOM anyway.

annevk · 2016-10-26T08:38:38Z

I think this is ready to be merged now, though it would be nice to hear from @Hixie why the obvious code is wrong.

domenic · 2016-10-26T14:29:56Z

secno is needed for #27

annevk · 2016-10-26T15:10:16Z

Shall we add that as part of fixing that, then? Could even only output it in that mode as that doesn't have single-page. Would save about 700 nodes for normal HTML single-page.

domenic · 2016-10-26T15:34:16Z

Sure, works for me. In that case I'll review/probably merge this PR later today.

annevk · 2016-10-28T08:02:24Z

Thanks @Hixie, made some changes based on that. I'm not sure I totally follow with regards to stack/heap, but enough to remove a redundant GetAttribute call (although I guess those must be very cheap).

Hixie · 2016-10-28T18:01:34Z

src/wattsi.pas

@@ -637,7 +636,6 @@    TCrossReferences = record
                        begin
                           SecondLI := NewLI.CloneNode(True);
                           ExtractedData := EnsureID(NewLI, 'toc-' + LastSeenHeadingID);
-                           Assert(not ExtractedData.IsEmpty);


Why did you remove this assert?

EnsureID has it, no?

The point of the assert here is to make sure EnsureID doesn't break without you noticing. :-)

Hixie · 2016-10-28T18:01:57Z

src/wattsi.pas

@@ -504,9 +505,7 @@    TCrossReferences = record

                  Scratch := Default(Rope);
                  Scratch.Append('#');
-                  ExtractedData := Element.GetAttribute('id');
-                  Assert(not ExtractedData.IsEmpty);


this assert also seems like an important one to keep...

Same as above. EnsureID already asserts.

EnsureID is verifying that things within EnsureID are acting as expected.
But here you're outside EnsureID, so you can't trust that EnsureID is correct. Hence the assert.

So should we add more asserts then? These were the only two places that did that.

Add asserts anywhere you have an invariant and you're not confident the rest of the code will keep it, for example when someone breaks the code elsewhere. :-)

Hixie · 2016-10-28T18:03:22Z

src/wattsi.pas

+
+                  Element.AppendChild(HeadingSelfLink);
+               end;
+


i would add a comment here saying ExtractedData is no longer valid past this point

But it can be assigned again, right? You did not add such comments for other AppendDestructively. Should we comment all of them?

It's only non-obvious here because you only destroy in one branch of the if block.

annevk · 2016-10-31T10:07:23Z

Added the asserts back and added a comment about the validity of ExtractedData. I think this should be good to now.

Corresponding stylesheet PR: whatwg/resources.whatwg.org#21

See whatwg/wattsi#30 for context.

Append a self-link to each heading

bd7c743

Fixes whatwg/html#46.

domenic requested changes Oct 24, 2016

View reviewed changes

Do not add section symbol; add self-link class

bc6ee4c

domenic added 2 commits October 24, 2016 15:13

Add heading class; remove extra space; add . after number; wrap number

bc87eaf

in <span class="secno">

Remove extra period

ca09c7f

Bikeshed doesn't include it in the TOC, but does in the document, so don't even bother trying to be consistent with Bikeshed, since it's inconsistent with itself.

annevk self-assigned this Oct 25, 2016

annevk added a commit to whatwg/resources.whatwg.org that referenced this pull request Oct 26, 2016

Make self-link work for the HTML Standard (headings only for now)

933b5d6

See whatwg/wattsi#30 for context.

annevk mentioned this pull request Oct 26, 2016

Make self-link work for the HTML Standard (headings only for now) whatwg/resources.whatwg.org#21

Merged

drop heading class

4cbeba9

drop HeadingSecno too

76fe951

annevk commented Oct 26, 2016

View reviewed changes

adjust per feedback (also remove redundant assert)

b364578

Hixie reviewed Oct 28, 2016

View reviewed changes

restore asserts / add comment

0222e95

domenic merged commit 1572d7e into master Oct 31, 2016

domenic deleted the annevk/self-link branch October 31, 2016 18:51

domenic pushed a commit to whatwg/resources.whatwg.org that referenced this pull request Oct 31, 2016

Make self-link work for the HTML Standard (headings only for now)

9621d44

See whatwg/wattsi#30 for context.

domenic mentioned this pull request Apr 18, 2017

Wrap heading section numbers in <span class=secno> #44

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Append a self-link to each heading #30

Append a self-link to each heading #30

annevk commented Oct 24, 2016

domenic left a comment

domenic commented Oct 24, 2016

domenic commented Oct 24, 2016

domenic commented Oct 24, 2016

annevk commented Oct 25, 2016

domenic commented Oct 25, 2016

annevk commented Oct 25, 2016

domenic commented Oct 25, 2016

annevk commented Oct 25, 2016

annevk commented Oct 25, 2016

domenic commented Oct 25, 2016

annevk commented Oct 25, 2016

annevk commented Oct 26, 2016

annevk Oct 26, 2016

Hixie Oct 27, 2016

Hixie Oct 27, 2016 •

edited

Loading

Hixie Oct 27, 2016

annevk commented Oct 26, 2016

domenic commented Oct 26, 2016

annevk commented Oct 26, 2016

domenic commented Oct 26, 2016

annevk commented Oct 28, 2016

Hixie Oct 28, 2016

annevk Oct 28, 2016

Hixie Oct 28, 2016

Hixie Oct 28, 2016

annevk Oct 28, 2016

Hixie Oct 28, 2016

annevk Oct 28, 2016

Hixie Oct 30, 2016

Hixie Oct 28, 2016

annevk Oct 28, 2016

Hixie Oct 28, 2016

annevk commented Oct 31, 2016

Append a self-link to each heading #30

Append a self-link to each heading #30

Conversation

annevk commented Oct 24, 2016

domenic left a comment

Choose a reason for hiding this comment

domenic commented Oct 24, 2016

domenic commented Oct 24, 2016

domenic commented Oct 24, 2016

annevk commented Oct 25, 2016

domenic commented Oct 25, 2016

annevk commented Oct 25, 2016

domenic commented Oct 25, 2016

annevk commented Oct 25, 2016

annevk commented Oct 25, 2016

domenic commented Oct 25, 2016

annevk commented Oct 25, 2016

annevk commented Oct 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hixie Oct 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk commented Oct 26, 2016

domenic commented Oct 26, 2016

annevk commented Oct 26, 2016

domenic commented Oct 26, 2016

annevk commented Oct 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk commented Oct 31, 2016

Hixie Oct 27, 2016 •

edited

Loading