Conversation
| // If we have amp-fit-text on, then reduce the font size to prevent the block | ||
| // shifting due to font size not fitting. | ||
| if ( ampFitTextElement ) { | ||
| while ( appliedWidth < scrollWidth || appliedHeight < scrollHeight ) { |
There was a problem hiding this comment.
Looks like the sliding issue came from the initial pre-resize amp-fit-text font size being too large for the container and this causing the container to "slide". After resizing stopping and the font calculation happened, the block "jumped" to its correct position. Note that this did not influence the top/left positioning in any way.
WDYT of this approach? Reducing the font size already while resizing.
There was a problem hiding this comment.
If it's performant, it's a great idea and better UX than the original. Does it work well for even long texts and weird fonts (with many ligatures)?
There was a problem hiding this comment.
It only works when making the block smaller, not when making it larger, which is confusing. Also doesn't seem to respect MAX_FONT_SIZE and MIN_FONT_SIZE. I just made a block with lots of text very small and the text basically disappeared because the font size was set to 0px.
There was a problem hiding this comment.
It definitely should have font limits. Also, if we go that way we should also implement the same logic for the post meta blocks as well to make it consistent.
On performance: seems to work well with weird fonts as well, at least as far as I tested.
On working when making the block smaller only: yep, I was doubting that it might be confusing. When turning off amp-fit-text and the previously saved font size is too large then we automatically increase the size of the block so that the text would fit inside. However, we do not reduce the block if there's a lot of room. Meaning that elsewhere we do care about the font not going over the lines, not so much about there being extra room left. Because of that, I was 50-50 that perhaps it's OK just to make sure that the text fits the block but ignore if there's more room. It's a bit different case for amp-fit-text though.
Anyway -- I'll add the size increasing as well now that you brought it out as well.
|
I see quite a few problems with this though. I have the default fitted text box on a new story, paste a lot of text in there and start resizing. I can't make the box shorter, when I make it narrower the content jumps weirdly around, when I make it wider the content font size is suddenly tiny. All this is without rotating. In the end I ended up with this weird monstrosity - where the content doesn't take up the full width while at the same time taking up too much length. |
|
Thanks for testing. Yes, it's not final yet. Was working on this but wasn't able to figure out yet why some of the things were happening as they were happening so will continue tomorrow morning. Just pushed a fix for the specific issue that you mentioned, however, there are still a bunch of oddities happening. |
|
Related to some of the issues I'm seeing: #3398 Not sure exactly why that happens. |
|
An alternative for fixing the "sliding" specifically would be to assign Using
Anyway, will give it another try in the morning. |
|
After looking into this more then it looks like it does not make sense right now to add the font size reducing -- mostly due to the bug #3398 which means that while resizing the text is quite often switching between tiny and huge font so it looks like some odd flickering. That would need fixing first first. Also tried reducing/increasing the font by 1 instead of trying to find the "ideal" font size but this causes some additional issues when resizing very fast. |
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
@googlebot I consent. |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
|
@miina, this is once again due to the slightly weird decision of displaying the inner box as When the overflow changes to I have now set the We really should place that inner block correctly with |
|
Thanks for this! Tested with Text block and post meta blocks (title, author, date), with both amp-fit-text on and without; with multiple lines and single lines; also with bold and italic in the middle in case of the Text block. Didn't detect any regressions. |
|
@spacedmonkey As you haven't touched the code of this PR, would you mind reviewing? |
swissspidy
left a comment
There was a problem hiding this comment.
Just tested with text block (with amp-fit-text on) and it still is sliding: https://cloudup.com/cfIG3pOXUBP
|
I can confirm that I am still seeing the slide as well :( |
|
Uh, that's unexpected. Will debug some more then. |
|
Weird but I'm not able to replicate it, tried for some time with different rotations etc.
What I did:
Is there anything else you're doing when testing? Not sure what am I doing differently. |
|
@miina Hmm not seeing the exact same issue as in the video anymore, but it still happens when making the block very small or very big. See new video: https://cloudup.com/cn74fKgoQZL Comparing with your demo, it doesn't go as far. |
|
@swissspidy Thanks for checking again. On the block sliding when going very large: that seems to be a different issue, looks like it's related to the resizing logic + positioning and not the font size. We might want to report a separate issue for that for prioritization. Potentially it's less important since it's unlikely to resize the block that much over the limits of the Page. Thoughts? So many issues with resizing have appeared lately. On the block sliding when going very small: |
|
These two are related in terms of being related to resizing but have a different cause. #3444 still needs investigation and Implementation Brief but it's likely related to positioning the block while resizing. #3461 has Implementation Brief where the source of the issue is mentioned (see Cause of the bug and possible fix). In this issue's case, it's the font size that causes it. Note that #3444 might be related to sliding when making a block very big since both are related to positioning the block when resizing. We could combine those if you prefer! |
|
Thanks for the clarification. I'm fine with merging this for now as it's already an important improvement. It would be good to take a more holistic approach for the remaining resizing issues, otherwise we keep chasing edge cases. |
|
Most of the resizing bugs so far haven't been the quickest to resolve, and somehow each bug has brought out new bugs. Initially, I combined the newly found related bugs as well but then it started taking too much time since the issues (causes) were actually different + each one of those took some debugging time. And then one bug was fixed but waiting for another bug getting fixed, too. Also, some cases are edge cases and some more common cases, so prioritization would be good, too. Not sure what's the best approach here, not sure if putting all the issues together would make sense in terms of having different causes + taking too much time to get anything fixed. 🤷♀At least that's how I felt when actually working on those. |
I am not sure I agree here. A fix for one issue, may effect other work another developer is working on. |
The library is just fine. The problem lies in the complexity of resizing elements that are rotated, as we have to do re-calculate the block's top and left position constantly while resizing. |
In my case, I had been debugging one issue for a few days. Then another issue came out which had a completely different cause. That took quite a long time to figure out + there was some feedback rounds and it delayed the first (related but not the same) bug for more than a week. If we gather all the different bugs together then it might take quite a long time for those getting fixed although another bug got fixed already. Also -- some of these bugs are edge cases (such as #3444) and being mindful of the timeline then we should prioritize.
Nope, not upstream bugs, so far there have been a lot of regressions since resizing is quite influenced by a lot of stuff and almost every block has resizing + we've added dragging and rotating to it. So it's fully our own bugs :) |
|
Just realized that there's a bug related to this PR: the fix should be for amp-fit-text only! Reopening |
Summary
From investigation:
The issue here is coming from the font size going over the limits of the block while reducing the size and this making the block slide. It does not have anything to do with positioning.
Addresses issue #3447
Checklist