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.

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

Use <details> element for collapsible sections instead of JS #677

Closed
ISNIT0 opened this issue Apr 17, 2019 · 30 comments · Fixed by #708
Closed

Use <details> element for collapsible sections instead of JS #677

ISNIT0 opened this issue Apr 17, 2019 · 30 comments · Fixed by #708
Assignees
Milestone

Comments

@ISNIT0
Copy link
Contributor

ISNIT0 commented Apr 17, 2019

For context: #633

If we are to re-visit the collapsible headings, it may make more sense to re-write the open/close logic to not depend on JavaScript at all using something like:

@cm8 @mossroy @Jaifroid

@ISNIT0
Copy link
Contributor Author

ISNIT0 commented Apr 17, 2019

I'm personally leaning towards using <details> because it's semantic, simple, and clear how it will behave if the browser doesn't support it (show all content)

@mossroy
Copy link

mossroy commented Apr 17, 2019

According to https://caniuse.com/#search=details , some polyfills exist for browser that don't support this html tag. In particular https://github.com/javan/details-element-polyfill

@ISNIT0 ISNIT0 changed the title Discussion: Modify article section collapsing logic Use <details> element for collapsible sections instead of JS Apr 29, 2019
@ISNIT0 ISNIT0 self-assigned this Apr 29, 2019
ISNIT0 added a commit that referenced this issue Apr 29, 2019
@ISNIT0
Copy link
Contributor Author

ISNIT0 commented Apr 29, 2019

This is implemented in #708

You can see an example ZIM file here:
https://framadrop.org/r/cYeF-hsrXN#UWwvdi4IR2uEx/z7JkzgSYLUrlObAgrjQml0zUOLmIA=

I've tested it on KiwixJS and Kiwix Mac. It seems to be working properly.
I think we can modify the appearance slightly, but I guess it's best if we use the native UI?

@Jaifroid
Copy link
Collaborator

@ISNIT0 There's no support for details in Edge, which is the browser engine on which Kiwix JS Windows depends (EdgeHTML is the supplied engine in UWP apps for now), and one of the supported browsers in Kiwix JS. However, as @mossroy says, we could include a polyfill instead; or continue to use our existing workaround if the polyfill is cumbersome.

However, do we really want to introduce new HTML into the document? One thing is providing custom CSS or JS files, but another is changing the HTML. Changing all H1 or all H2 (and H3?) headings to details is going to disrupt a lot of existing code in ZIM readers: for example, code that creates Tables of Content. Of course that code can be rewritten, but it will have to include legacy support for old ZIMs.

I wonder why Wikimedia's own solution doesn't work for us? It would be so much easier than injecting CSS or JS or altering the HTML quite so radically.

@ISNIT0
Copy link
Contributor Author

ISNIT0 commented Apr 29, 2019

@Jaifroid Sorry, I forgot to update here, I've implemented the polyfill here: #708

You can download the updated ZIM file from there :)

@Jaifroid Your comment about removing the hs is something that I hadn't thought about quite enough - you're right.
I decided not to put the h tags inside the <summary> elements because an MWOffliner dependency (Domino) doesn't handle it very well, but it's definitely worth working through for the semantic consistency

@Jaifroid
Copy link
Collaborator

Thanks! I'll test.

I've noticed one issue (testing on Chromium) with the framadrop ZIM linked in this thread, which is that clicking on a footnote doesn't open the "Notes" section (if it's closed), and therefore there is no jump to the relevant note. Tested in Kiwix JS in Service Worker mode. I understand this is more a proof-of-concept for testing (?) so that part of course isn't implemented yet, but I presume that would still require custom JavaScript?

@ISNIT0
Copy link
Contributor Author

ISNIT0 commented Apr 29, 2019

@kelson42
Copy link
Collaborator

@ISNIT0 So far I can see:

  • cursor on paragraph title is wrong
  • Behaviour is not the same as current: paragraph are not expended in desktop view

@mossroy
Copy link

mossroy commented Apr 29, 2019

I quickly tested the framadrop file of @ISNIT0 last comment.
On FirefoxOS, it correctly falls back to open sections (because it does not support the tag, and the jQuery mode does not handle javascript for the polyfill).
I confirm the 2 problems mentioned by @Jaifroid.
On Firefox 66, there is no arrow icon next to the section title (there is one on Chromium 73).
On both browsers, in ServiceWorker mode, a click on a footnote correctly jumps to the note. It does not work in jQuery mode (except if the section of footnotes is already open), but that's something that might be fixed in kiwix-js.

@Jaifroid
Copy link
Collaborator

@ISNIT0 For reasons of compatibility with code that creates a Table of Contents dynamically, would it be possible to attach the id of each heading to the h1 h2 h3 tags, instead of to the details tag? A quick test in Chromium (editing the HTML live in the Dev tools) doesn't seem to affect the styling. See screenshot of the (original) HTML I am referring to:

image

In this example, we would move the id="History" from details to h2.

Other things may depend on the ID being in the h1 h2 h3 tags rather than in details, e.g. any custom JavaScript to open or close headings in browsers that don't recognize details. We have to support legacy ZIMs as well as these new ZIMs, and anything that keeps the structure of documents in the two ZIM types as close as possible will help reduce the need for coding special cases and exceptions.

@Jaifroid
Copy link
Collaborator

As a followup to the above, here is an example of the same code from the same article in a "legacy" ZIM (wikipedia_en_all_novid_2018-06):

image

In short, it would be great if the change merely substituted details for section tags, but otherwise left the h1 h2 h3 intact and fully functional for clients that don't understand details.

@ISNIT0
Copy link
Contributor Author

ISNIT0 commented Apr 30, 2019

@kelson42 What do you mean by "cursor on paragraph title is wrong"?

Thanks for your feedback everyone, these are the tasks I think I need to do:

  • Check paragraph expanding
  • Use custom arrow icon for all summary tags
  • Move id from details to h tags
  • Change cursor to "hand"

@kelson42
Copy link
Collaborator

@ISNIT0 The mouse pointer is wrong, should be the "hand", but is the "cursor".

@ISNIT0
Copy link
Contributor Author

ISNIT0 commented Apr 30, 2019

@kelson42
Copy link
Collaborator

@ISNIT0 Looks good to me
@Popolechien Might make sense for you to have a look too!

@Popolechien
Copy link

Looks good to me - both on desktop and smartphone!

@kelson42 kelson42 added this to the 1.9 milestone Apr 30, 2019
@Jaifroid
Copy link
Collaborator

Hi @ISNIT0, with this new method, the following line in article_list_home.js is causing an exception:

document.getElementsByClassName('mw-body-content')[0].classList.remove('nojs');

The exception is "Unable to get property 'classList' of undefined or null reference". Maybe do something like this:

var mwBodyContent = document.getElementsByClassName('mw-body-content');
if (mwBodyContent[0]) mwBodyContent[0].classList.remove('nojs');

Maybe related to the removal of some classes in this type of ZIM?

@mossroy
Copy link

mossroy commented Apr 30, 2019

It works much better, indeed.
But the section are always closed by default, even on desktop (where they were previously open) : is it on purpose? If find it less convenient on large screens.

I'm also wondering if it's good to close the subsections by default. For example, in article "Beaune Altarpiece" where there are many sections and subsections, I find it difficult to see in which subsection we are, and to open everything if I'd like to read the whole article.
Maybe only the top level sections could be closed by default? The mobile online version of wikipedia seems to work like that.

@ISNIT0
Copy link
Contributor Author

ISNIT0 commented May 1, 2019

@mossroy Interesting, the sections seem to automatically open for me on desktop (window.innerWidth > 720) - I have now changed the way the sections are opened (using attributes, rather than click events, which may resolve your issue).
I've also now made it open only the TOC level 2 sections on desktop

@Jaifroid I've updated article_list_home.js to check what context it's being used in and behave appropriately

You can see the generated ZIM here: https://framadrop.org/r/D1EE0C6YxL#SwJO6719lYGfukNN1i71HHy1glAK4MaJTdKiifDHBlo=

@Popolechien
Copy link

@ISNIT0 Confirm it works on macOS (10.14) here too. Pretty neat.

@Jaifroid
Copy link
Collaborator

Jaifroid commented May 1, 2019

@ISNIT0 Below is what I'm seeing by default on Chromium (Kiwix JS, Desktop, Service Worker mode [i.e. with JavaScript fully supported]). As you can see, the h2 sections are all open, but h3 is closed. When I narrow the screen, to less than 720 pixels width, the h2 sections do not close. Are you seeing the same? Is this behaviour as intended?

image

@Jaifroid
Copy link
Collaborator

Jaifroid commented May 1, 2019

Qualification to above: if I restrict the width of the screen <720 before I open the article, then all sections are closed. However, changing the width of the browser window while the article is open does not trigger closing of the sections (which was the previous behaviour). That may not be important.

@ISNIT0
Copy link
Contributor Author

ISNIT0 commented May 1, 2019

@Jaifroid I don't think it has ever responded to resizes in this way.
This is intended behaviour 👍

ISNIT0 added a commit that referenced this issue May 1, 2019
✨ Progress on #15 and implemented #677
@Jaifroid
Copy link
Collaborator

Jaifroid commented May 1, 2019

@ISNIT0 The behaviour with previous ZIMs (e.g. any previous WikiMed from 2018) is that the sections are closed by the stylesheet rules if the screen is less than 720px, therefore changing the window size changes the behaviour dynamically. It's probably not important. The only case where it might be important is if the user changes the orientation of their screen in mobile. Old behaviour was that in portrait mode, sections are closed, in landscape mode, sections are open. Anyway, it's not a big deal.

Thank you for your hard work with this change and for the creative solution!

@kelson42
Copy link
Collaborator

kelson42 commented May 7, 2019

@ISNIT0 I don't see it in https://download.kiwix.org/zim/other/rationalwiki_en_all_novid_2019-05.zim for example (no small arrow on the left of each paragraph title). And this ZIM file has been done with 1.8.6.

@kelson42 kelson42 reopened this May 7, 2019
@ISNIT0
Copy link
Contributor Author

ISNIT0 commented May 7, 2019

@kelson42 Perhaps this is the same issue I had with my UATs when I had to reinstall it again.
Does a clean install (remove && install) fix the issue?

@kelson42
Copy link
Collaborator

kelson42 commented May 7, 2019

@ISNIT0 All of this is Docker... it is anyway reset.

@ISNIT0
Copy link
Contributor Author

ISNIT0 commented May 7, 2019

Hmmm, I'll look into it.

@ISNIT0
Copy link
Contributor Author

ISNIT0 commented May 7, 2019

@kelson42
docker run openzim/mwoffliner:1.8.6 mwoffliner --version gives 1.8.5
Looks like the docker image compilation has gone wrong

@ISNIT0
Copy link
Contributor Author

ISNIT0 commented May 7, 2019

Caused by #722

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants