llsoftsec / llsoftsecbook

Low-Level Software Security for Compiler Developers
https://llsoftsec.github.io/llsoftsecbook/
Other
527 stars 50 forks source link

Make HTML output look and feel better #130

Open kbeyls opened 1 year ago

kbeyls commented 1 year ago

The current HTML version of the book can be improved a lot.

In this issue, let's record an overview of all the ideas for improvements. More detailed discussions for specific improvements can be done in separate issues when that makes sense.

Overall, there are a number of examples of HTML books that show us examples of how nice and functional HTML books can look. Let's take inspiration from them. At the moment, I think we have at least 3 examples:

  1. https://bookdown.org/, especially it's 3-column bootstrap-based design: https://bookdown.org/yihui/bookdown/html.html#bs4-book
  2. https://rust-lang.github.io/mdBook/
  3. https://jez.io/tufte-pandoc-css/ (especially the sidenotes feature)
kbeyls commented 1 year ago

A non-exhaustive list of good ideas from the examples above and improvements that probably also would be good for llsoftsecbook are:

kbeyls commented 1 year ago

A first set of improvements landed in #135

JLouisKaplan-Arm commented 1 year ago

The new PR looks great!

Now that I had a chance to use it interactively, I just had two thoughts.

edit: Just found these which might be a more bookdown idiomatic way of changing this: 1 and 2

happy to investigate a PR for either/both of these issues if helpful.

JLouisKaplan-Arm commented 1 year ago

Another thing I noticed testing on mobile is that the hamburger button doesn't work with JS disabled. I think this probably OK but thought it worth mentioning as we had previously discussed to what extent the book should cater to non-JS book viewers in #64

kbeyls commented 1 year ago

Thank you for trying out the new design, @JLouisKaplan-Arm !

* when the screen is large enough for the sidebar to be visible by default, the hamburger icon to toggle it could be removed. As well as being more conventional UI design, this would circumvent the current issue where, on large layout, pressing the button shifts the main content and scroll bar to the left in an unexpected way. Not sure how this works in bookdown but if it needs manual CSS we should be able to select the button's class/ID and mark it hidden for large layout

I also noticed this morning that the scroll bar shifts left when removing the TOC sidebar on large screens. I think that definitely needs to get fixed. I do think that there is still value in being able to remove the TOC sidebar on large screens too. Some reader might prefer to not see the TOC all the time so that they get less distracted by it? https://rust-lang.github.io/mdBook/ allows doing this, but the bootstrap-based bookdown (e.g. https://mastering-shiny.org/basic-reactivity.html) does not seem to allow that.

With bootstrap, I think it should be trivial to implement hiding the hamburger icon on large screens only by adding the appropriate bootstrap class to it. But as said above, I think it's better to keep giving the user the option to hide the sidebar also on large screens.

* on all small screens I tested (desktop chrome/firefox with small window, mobile firefox) I was able to scroll horizontally into areas with no content. If I remember correctly, I put the `overflow-x: hidden;` line into CSS to stop this happening which was removed in [7665583](https://github.com/llsoftsec/llsoftsecbook/commit/76655835928ce344016689c9c9037986f66bc9f1) - I did not mention it in review as I thought bookdown might handle it in some other way but looks like we could consider adding it back in.

edit: Just found these which might be a more bookdown idiomatic way of changing this: 1 and 2

happy to investigate a PR for either/both of these issues if helpful.

I'd be delighted if you would create a PR for the second issue you see. For the first issue, maybe we first need to get to a conclusion on whether it's useful to give the user an option on large screens to remove the TOC? (That scroll bar shifting is indeed something that shouldn't happen when doing so).

kbeyls commented 1 year ago

Another thing I noticed testing on mobile is that the hamburger button doesn't work with JS disabled. I think this probably OK but thought it worth mentioning as we had previously discussed to what extent the book should cater to non-JS book viewers in #64

Yes, I could not find a way to make the hamburger work as desired without using a bit a javascript, I'm afraid.

JLouisKaplan-Arm commented 1 year ago

I do think that there is still value in being able to remove the TOC sidebar on large screens too. Some reader might prefer to not see the TOC all the time so that they get less distracted by it?

Yes that makes sense to me. Definitely nice to keep the option if possible.

I'd be delighted if you would create a PR for the second issue you see.

Sounds good - will try and give it a go.

a way to make the hamburger work as desired without using a bit a javascript

It is possible with a fair amount of CSS hacking (explored this a while ago for a personal website) but I have no idea if it will be compatible with the rest of the bookstrap approach. Personally I think the featurefullness of JS makes using it here worthwhile so I'm happy as it is.

kbeyls commented 1 year ago

I just put up #136 to try to fix the position-of-scrollbar issue on large screens discussed above.

JLouisKaplan-Arm commented 1 year ago

A few more thoughts after trialling the new UI

I'm really being fussy here though. I think it looks fantastic already.

kbeyls commented 1 year ago

A few more thoughts after trialling the new UI

* Do you think it would be nice if (on medium + large screens) the central text stayed centered even when the contents are toggled away?

I'm not fully sure, but I think you mean that if you make the TOC invisible, the main text does not move to a different position? Currently, the text stays centered on the space it has available on the screen, which is less if the TOC is visible. This could be changed, but I think it would result in the main text always being centred as if the TOC is present. If the TOC is then not present, it would seem the main text is not centered. (I hope this explanation makes sense. I could add some screenshot/mockup to make it more clear). Or did I misunderstand your suggestion?

* While we are centering things, do you think we should center some of the narrower figures(again, on medium/large screens) as some they currently left aligned

Yes, we should. Let me add a "todo" on a higher up comment which lists open todos.

* The alt text that appears on mouse hover are great, but on buttons to the left, the alt text obscures buttons to the right of it. I'm wondering if there's a way to get this pop-up to appear below the mouse rather than to the right of it.

Agreed, this has annoyed me too. I've tried to (partially) address this in #141.

I'm really being fussy here though. I think it looks fantastic already.

Thank you for being fussy. I think we need to be fussy to make it look fantastic ;)

JLouisKaplan-Arm commented 1 year ago

Currently, the text stays centered on the space it has available on the screen, which is less if the TOC is visible.

Ah right I see now that it does behave that way. I was viewing parts of the book without any sidenotes (and with TOC disabled) so it seemed like the main text was aligned off-center slightly to the left. But now seeing the sections with side notes I see that it is centered when considering (text + side notes) as a column altogether. Hope that makes sense. As for one or the other approach, I have no preference :)

A last thing I noticed today - is it expected that when first landing on the page (before clicking any sections or toggling TOC) and scrolling down, that there is a floating vertical scroll bar for the main content? (I.e. it's not 'attached' to the edge of the viewport.) This seems to disappear with a toggle/re-toggle of the TOC, after which the vertical scroll bar moves to where I would expect it at the rightmost edge of the viewport.

kbeyls commented 1 year ago

A last thing I noticed today - is it expected that when first landing on the page (before clicking any sections or toggling TOC) and scrolling down, that there is a floating vertical scroll bar for the main content? (I.e. it's not 'attached' to the edge of the viewport.) This seems to disappear with a toggle/re-toggle of the TOC, after which the vertical scroll bar moves to where I would expect it at the rightmost edge of the viewport.

I also noticed a similar issue when first opening the page in Chrome yesterday. On a large screen, I noticed the width of the sidebar and the main text somehow being wider than the viewport, and that resulting in seeing the TOC sidebar centered, and below it, if you scroll, the main text. Not sure if it's the same issue as what you're seeing. I'll try to do a few experiments today to see where the issues shows up (is it browser-specific? OS-specific? does it happen at all screen sizes?)

Update: I now tried it with a number of browsers on Windows, OSX and linux. It seems this issue only shows up on Windows, across all browsers I tried (Firefox, Chrome, Edge).

g-kouv commented 1 year ago

A bit late to the party, but I'm wondering if we need the tooltips at all on mobile. To make them appear you need to already click on the button. I don't particularly mind the tooltips for the repo button and "suggest an edit" button, even though they will only appear shortly after you click on them, before the browser navigates to their target, and then again if you navigate back (when they are "sticky").

But the TOC tooltip is sticky even though you press the button which has clearly toggled the TOC (so the tooltip doesn't give any new information), and keeps hiding the other two buttons. To get rid of it, you need to click somewhere else. WDYT?

kbeyls commented 1 year ago

A bit late to the party, but I'm wondering if we need the tooltips at all on mobile. To make them appear you need to already click on the button. I don't particularly mind the tooltips for the repo button and "suggest an edit" button, even though they will only appear shortly after you click on them, before the browser navigates to their target, and then again if you navigate back (when they are "sticky").

But the TOC tooltip is sticky even though you press the button which has clearly toggled the TOC (so the tooltip doesn't give any new information), and keeps hiding the other two buttons. To get rid of it, you need to click somewhere else. WDYT?

Yeah, not having the tooltip on interfaces where you use fingers rather than a mouse make sense to me. I'd have to investigate if there is a way with CSS to figure out if the interface is touch-driven or mouse-driven to be able to implement it...

g-kouv commented 1 year ago

Maybe something to change in javascript instead? Where we initialise tooltips, for example:

let tooltipList = [...tooltipTriggerList].map( tooltipTriggerEl => new bootstrap.Tooltip(tooltipTriggerEl, {trigger : 'hover'}));

kbeyls commented 1 year ago

Maybe something to change in javascript instead? Where we initialise tooltips, for example:

let tooltipList = [...tooltipTriggerList].map( tooltipTriggerEl => new bootstrap.Tooltip(tooltipTriggerEl, {trigger : 'hover'}));

There's a PR for this now at #144 .

kbeyls commented 7 months ago

There's now a PR for adding a download button at #235