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

Heading Block: Inconsistent location of HTML Anchor in Advanced section of sidebar #7964

Closed
bph opened this issue Jul 14, 2018 · 11 comments
Closed
Labels
[Feature] Block API API that allows to express the block paradigm. [Priority] Low Used to indicate that the issue at hand isn't a top priority to address and can be handled later [Type] Bug An existing feature does not function as intended
Milestone

Comments

@bph
Copy link
Contributor

bph commented Jul 14, 2018

Describe the bug
When accessing the "Advanced" section for the first time the "HTML Anchor" item is in second place. On subsequent views, the HTML Anchor section is the first slot.

To Reproduce
Steps to reproduce the behavior:

  1. Create a new post
  2. Add three paragraphs blocks, the second one be only one line
  3. select the second one (and also open the Advances settings section in the sidebar)
  4. Transform the paragraph to a Heading via the toolbar button
  5. Add a anchor item in the sidebar (second field in advanced settings)
  6. unselect block
  7. Select it again
  8. Open Advanced again

Expected behavior
I expect the location of the HTML Anchor to stay in the same place in the menu

Screenshots
Here is a video: http://recordit.co/bpv6h1VtIu

Desktop (please complete the following information):

  • OS: 10.13.5 OS X
  • Browser Firefox Developer Edition
  • Version 62
    Also reproducible in Google Chrome
    --- OP edited with new instructions and video 7/23 --bph
@bph bph changed the title Heading Block: Inconsistent location of the items in Advanced section of sidebar Heading Block: Inconsistent location of HTML Anchor in Advanced section of sidebar Jul 14, 2018
@designsimply designsimply added [Feature] Blocks Overall functionality of blocks [Status] Needs More Info Follow-up required in order to be actionable. labels Jul 17, 2018
@designsimply
Copy link
Member

designsimply commented Jul 17, 2018

I was unable to reproduce this in testing because the Heading block > Advanced panel > Anchor HTML field was always first in my testing:

Video: 38s
Seen at http://alittletestblog.com/wp-admin/post.php?post=13995&action=edit running WordPress 4.9.7 and Gutenberg 3.2.0 using Firefox 61.0.1 on macOS 10.13.5.

heading-block-advanced-panel-html-anchor-order-test

@bph I'm closing this issue for now because I was unable to reproduce the problem as described. Totally happy to re-open anytime tho if there's something I've missed (!) or if we can come up with a way to consistently reproduce the issue. If you can still reliably reproduce the problem, could you confirm for me which version of Gutenberg you are testing with and also try a complete clearing of cache and cookies and re-testing to see if the reason you're seeing a strange ordering of options is from some kind of stuck local cache?

@bph
Copy link
Contributor Author

bph commented Jul 17, 2018

Yes, I took me a while to actually pin down why I was so irritated by the different locations of the HTML Anchor field. It's not always and definitely one attempt to reproduce it won't let you see it.
When you watch the video, you see that I first created the header and when I went back to add the anchor. You might need to try different variations yourself to reproduce it. My tests today, also didn't reveal any inconsistencies. It's not the first time, with this issue I thought I had a reproduce-save scenario when I filed it. But I guess, it's gone away again. Thanks for testing it...

@designsimply
Copy link
Member

Okay! Thanks for commenting (and for submitting a well-written issue!) and if you think of anything else to try or if it becomes a more consistent problem for you, please comment here again and I will see it and give it another round of testing. fwiw, I did try several attempts to reproduce when I tested earlier, but only in the one session/browser.

@bph
Copy link
Contributor Author

bph commented Jul 23, 2018

@designsimply - This time, I think, I got the right combination.
It happens the moment you convert an one line paragraph into a heading.

Here is a video in Google Chrome
http://recordit.co/bpv6h1VtIu

I am able to consistently reproduce this behavior in Chrome and also in Firefox.
-- edited OP with instructions and new video as well. 7/23

@designsimply
Copy link
Member

Re-tested with the updated steps and confirmed that the Heading Block > Sidebar > Advanced > HTML Anchor text box appears 2nd only immediately after the block is transformed into a heading block and then the HTML Anchor text box appears 1st every time the block settings are accessed from that point forward.

@designsimply designsimply reopened this Jul 23, 2018
@designsimply designsimply added Good First Issue An issue that's suitable for someone looking to contribute for the first time [Feature] Block Transforms Block transforms from one block to another [Type] Enhancement A suggestion for improvement. and removed [Feature] Blocks Overall functionality of blocks [Status] Needs More Info Follow-up required in order to be actionable. labels Jul 23, 2018
@danielbachhuber danielbachhuber added [Type] Bug An existing feature does not function as intended [Feature] Block API API that allows to express the block paradigm. and removed [Feature] Block Transforms Block transforms from one block to another Good First Issue An issue that's suitable for someone looking to contribute for the first time labels Jul 25, 2018
@danielbachhuber
Copy link
Member

I saw this happening yesterday with a custom block I was working on. Essentially, the order of my controls varied randomly.

Refiling as [Type] Bug.

@danielbachhuber danielbachhuber removed the [Type] Enhancement A suggestion for improvement. label Jul 25, 2018
@mtias mtias added the [Priority] Low Used to indicate that the issue at hand isn't a top priority to address and can be handled later label Oct 27, 2018
@mtias
Copy link
Member

mtias commented Oct 27, 2018

@gziolo I remember discussing one of these around order of slots / fills.

@gziolo gziolo modified the milestone: WordPress 5.0 Oct 30, 2018
@gziolo
Copy link
Member

gziolo commented Oct 30, 2018

We had a similar issue in the past but we thought we solved it. @nosolosw is refactoring slots / fills to use new Context API from React in #11123. Let's see how it looks after this change is ready and try to fix it. I think it's so hard to reproduce that we can leave it after 5.0.

@oandregal
Copy link
Member

oandregal commented Oct 31, 2018

Took a quick look at this - thanks Birgit for the clear, reproducible instructions.

#11123 doesn't fix this because it's a pure refactoring: it makes SlotFill to use the new Context API without changing external behaviour.

This is the problem: the SlotFill component relies on loading component order to render the Fills. It follows a FIFO mechanism that uses occurrences to track the order. When the block declares an anchor, the Anchor Fill is loaded before the Additional class Fill (see). In this case, though, because we're transforming a paragraph to a heading, it looks like the Slot is not unmounted, so the Advanced Fill is added first (with its occurrence being i), and then the Anchor Fill is dynamically loaded (with its occurrence being higher than i). On subsequent loads (like selecting a different component and then selecting back the heading) they're loaded in the normal order.

  • Good news: Make anchor always render after Additional CSS class #11299 is a quick fix for this particular issue. It makes the anchor to load always after the additional CSS class. I'm not sure if there are components that can declare an anchor but not an additional class. If that's the case, we'll have the same problem but for Additional class. If that's not the case, the PR should be ok.
  • Bad news: the SlotFill mechanism doesn't play well with dynamically loading components because it uses a FIFO mechanism, the component itself doesn't declare an order or position. I'd say evaluating if this is worth changing and actually implementing it will take longer than we have for 5.0 and could probably be a nice exploration post-5.0.

@paaljoachim
Copy link
Contributor

Is this issue still valid?
If not let's close it.

@bph
Copy link
Contributor Author

bph commented Sep 14, 2020

When repeating the latest test case, it seems the location of the HTML Anchor field stays consistently in place. @paaljoachim Thanks for the prompt to re-test. Closing now.

@bph bph closed this as completed Sep 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block API API that allows to express the block paradigm. [Priority] Low Used to indicate that the issue at hand isn't a top priority to address and can be handled later [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

7 participants