Fix duplicate Page root element id

marcosvafilho commented 3 years ago

Issue:

Matestack by default renders two Page root elements, both containing a child div with the v-if directive, for instructing Vue.js on wether or not rendering the child element:

<div id="matestack-page-controller-action" class="matestack-page-root">
  <div v-if="asyncPageTemplate == null"> 
    <!-- content -->
 </div>
</div>

<div id="matestack-page-controller-action" class="matestack-page-root">
  <div v-if="asyncPageTemplate != null"> 
    <!-- content -->
 </div>
</div>

This used to be a non-issue, but now we've added an id attribute to the Page's root element, containing both the controller and the action, as we can see in the example above.

Although it doesn't appear to cause any actual problem right now, we cannot guarantee it won't cause in the future.

Also, it's not compliant with the HTML spec, which dictates the id attribute must be unique.

Changes

[x] Remove additional instances of the Page's root element.
[x] Make the now unique instance of the Page's root element to wrap both divs containing the v-if directive.
[x] Update the layout spec to reflect the new code.

Notes

Apparently, Matestack already has this issue with the global root element matestack-ui , which is added to both Pages and Apps. Since Apps can wrap Pages, we can have the situation of two elements with the very same id attribute. This PR does not fix this other issue, but I'll start a conversation with the maintainers about that.

jonasjabari commented 3 years ago

Thanks for the PR @marcosvafilho I will dive into that tomorrow and come back with some feedback :)

marcosvafilho commented 3 years ago

Thanks for the PR @marcosvafilho I will dive into that tomorrow and come back with some feedback :)

Thanks, @jonasjabari , no rush. I appreciate your feedback. Happy to improve whatever you identify as necessary.

jonasjabari commented 3 years ago

Hey @marcosvafilho

First of all: I realized there was an issue with your last PR #551 which the specs didn't cover and I oversee. This part https://github.com/matestack/matestack-ui-core/blob/next_release/lib/matestack/ui/core/page.rb#L18 is responsible for rendering only the page without the wrapping structure when performing a dynamic page transition (indicated by the query param only_page=true). Within this part the div id: page_id, class: 'matestack-page-root' is rendered together with the block (which is the response of the project specific page class). The resulting HTML string is then used by the Runtime template component (https://github.com/matestack/matestack-ui-core/blob/next_release/lib/matestack/ui/core/page.rb#L37) while performing the dynamic page transition. In your implementation (which is now on next_release), this Runtime template component itself is wrapped by div id: page_id, class: 'matestack-page-root' (https://github.com/matestack/matestack-ui-core/blob/next_release/lib/matestack/ui/core/page.rb#L36). This means that after a page transition this wrapper appears two times, which is of course not desired. So we need to get rid of the wrapping structure (https://github.com/matestack/matestack-ui-core/blob/next_release/lib/matestack/ui/core/page.rb#L36), which you actually did in the currently discussed PR (#554) We need to keep this removal!

Second: You moved the page_id to the first child of the div class: 'matestack-page-wrapper' trying to avoid rendering two divs with the same ID which is a good intention. But two things here: The page_id needs to be reevaluated on each page transition. It therefore can only be used in this part: https://github.com/matestack/matestack-ui-core/blob/next_release/lib/matestack/ui/core/page.rb#L18 as this is the only part which will get rendered on a page transition. The wrapping page structure does not get rerendered: Additionally the first child of the div class: 'matestack-page-wrapper' and the part responsible for page_only rendering https://github.com/matestack/matestack-ui-core/blob/next_release/lib/matestack/ui/core/page.rb#L18 are both rendering the matestack-page-root div which messes up the DOM Structure after a dynamic page transition.

I think all described issues (including your original duplicated ID issue) can be simply fixed by removing the wrapping div id: page_id, class: 'matestack-page-root' around the Runtime template component https://github.com/matestack/matestack-ui-core/blob/next_release/lib/matestack/ui/core/page.rb#L37 and rolling back all other changes made in this currently discussed PR

No duplicated IDs
same (visible) DOM structure on intial page load and after a page transition
Even after a page transition we don't have the duplicated ID issues as the v-if removes the not active DOM part completely. Therefore only one div id: page_id, class: 'matestack-page-root' will be present.
If the page is used without using Matestack's reactivity system based on Vue.js (which is totally possible), we're not facing the duplicated ID issue either. The Runtime template component would just be rendered as an empty div (which is not great but does not hurt that much as well)

Hope that makes sense. Probably not too easy to get your head around if your not too deep involved in the core implementation.

marcosvafilho commented 3 years ago

Hey @jonasjabari , thanks for this valuable feedback.

Yeah, a bunch of stuff to get my head around, but sure a fun task to do :)

I'll take some time to digest all the new information and then I get back to you.

Thanks again!

matestack / matestack-ui-core