Sometimes when changing the font family, "multiple" is shown as the font weight even if multiple font weights haven't been defined within the text.
It might be that the new font family doesn't have the font weight assigned and that causes it but this needs verifying.
Display the closest available font weight?
TBD by UX
Note: whatever is chosen should be displayed in the design panel and also for the text itself, too -- meaning applying it to the element content.
_Do not alter or remove anything below. The following sections will be managed by moderators only._
Note: UX Needed to decide if we should assign the closest available value or the default value if the previous font-weight is missing.
cc @barklund for visibility since you implemented processing the content of the text for getting the font weight -- perhaps there was already some discussion on what should happen if the new assigned font family doesn't have the previously assigned font weight.
There's a larger underlying issue here about actually changing the inline font weights to available ones when changing font rather than "just" showing another value in dropdown. This was left as an open question in #1323.
Updated the description for clarity to specifically mention that it should be applied to the text, too.
This also begs the question: What happens if the user tries to bold a text (using keyboard shortcut or button) with a font, that doesn't support bold? Should we disable the button even or do we use faux-bold?
CC @pbakaus, @samitron7.
/cc @o-fernandez
My vote goes to allowing 'faux bold' (system level bolding) as fallback for when there's no bold variant.
Let's say I have a text in Roboto supporting many weights and I have some text in there in all of these weights:
300
400
500
600
700
800
900
I then switch to a font, only supporting 300 and 400. What happens then? Do we add 700 as a "supported" weight even though the font doesn't support it and collapse all to the nearest font weight like so?
300 => 300
400 => 400
500 => 400
600 => 700
700 => 700
800 => 700
900 => 700
Alternatively we round down, so 600 => 400.
Or we don't support bold at all - for many grotesque display fonts faux-bold won't make sense anyway, so showing a button for it but not actually visually changing the font would be senseless.
We discussed this in a recent call and agreed to fixing this. No "multiple" shown, we'll just have to do approximations in some cases. When switching fonts it is ok to approximate to closest supported weight and/or default to "normal".
@miina "Multiple" is a bug. To be clear, no input field for a single selected element should ever have a "multiple" value. Multiple only appears when you select multiple elements that share a similar attribute like color, opactiy..etc.
And ditto to approximating then default to normal as a fallback
@miina "Multiple" is a bug. To be clear, no input field for a single selected element should ever have a "multiple" value. Multiple only appears when you select multiple elements that share a similar attribute like color, opactiy..etc.
@samitron7 Yes, this is a bug issue, reported to address the problem to ensure that "multiple" wouldn't appear unexpectedly. Will update the "Expected behavior" to mention that "no input field for a single selected element should ever have a "multiple" value".
Thanks Miina. I think I also filed a separate ticket a long time ago to change "multiple" to mixed because "multiple" is too long of a word to fit inside some input field and not sure where that came from >__<.
@barklund and I have been going back and forth a little in this PR to reach a clearer understanding of what this ticket should fix.
As of this comment: https://github.com/google/web-stories-wp/pull/4734#issuecomment-704347155 we reached agreement of how this should be handled. That essentially, when the font family updates, we want the font weight to return to 400 (regular) for the entire text element. If 400 is not an available weight for that new font family we want to get as close to it as possible (so either 300 or 500 and so on).
My draft PR does this for when you have the entire text element either selected (highlighted) or the text element is in focus. This however does not make it so that when you swap font families while your cursor is in the text element but the editor state has editorState.getSelection().isCollapsed or when just part of the text is highlighted that the font weight for the entire element is reset.
I think that this bug has turned into more of a feature, where we want to actually handle the reset of the font weight separately from just setting a new font weight for the text element because we need to trick the selection into being the entire text element so that we can strip any other font weights and then reapply the other styles for the text element that aren't related to the weight so that we can have consistent functionality on this reset no matter what part of the text element is selected or focused (my understanding is that we have different functionality set up for when a cursor is active in a text element, when part of a text element is highlighted and the text element is in focus).
After some discovery, it's pretty clear that we would need to alter how the fauxSelection updates or add in another update possibly to make the faux selection when resetting the weight the entire element or pass in an additional prop through the process of setting a new weight to tell functionality that it's a reset and to do something different in (probably) components/formatters/weight, components/panels/textStyle/useRichTextFormatting, components/richText/styleManipulation and components/richText/useSelectionManipulation.
Overall, this feels like a complete edge case and could possibly benefit from more discussion to figure out the best way to handle this to avoid more time getting sunk into it.
@bmattb @o-fernandez - thoughts on moving this to the backlog for the time being?
I don't think I fully follow, let me know if this is kind of true:
Is that roughly right? I may be misreading/missing important details here, so want to check. @barklund @BrittanyIRL
Hey @o-fernandez - thanks for summarizing, this is a sticky issue (my opinion!).
If you watch these two gifs on the comment i posted: https://github.com/google/web-stories-wp/pull/4734#issuecomment-704347155 you can see the difference.
Essentially yes, that second bullet point - but my understanding based off of talking to @barklund is that we want the same functionality regardless of where you are in the text selection which is reallyyyyy tricky to not create more bugs from in the process of overriding. But if it's ok that the font weight doesn't all go back to normal when the user swaps font families while actively in the text area then this is way easier. I think there's just not enough clarity on what the acceptance criteria for this actually is.
The original issue, what this ticket is trying to solve, is the scenario in which:
We want to avoid that scenario. Since there is actually just one font weight being used, not multiple, we shouldn't show multiple because the font wasn't resetting to a valid, available font weight.
In the GIF you added https://github.com/google/web-stories-wp/pull/4734#issuecomment-704347155 I see that the base case is that you do have multiple font weights in the font element. In that case, when you change the font family if you still have multiple font weights it's perfectly fine to have "multiple" in the font weight drop down, both before and after changing the font family. Ideally, we'd want font weights to be one of the valid font weights for that font family, but I don't think that's required for this bug.
So, to clarify acceptance criteria for this bug;
thanks for clarifying the acceptance criteria here, @o-fernandez - i think i have a working solution, i'm going to reexamine it first thing Monday and then I'll update the PR and clarify here that it's working as expected.
Hey @o-fernandez -
Thanks again for the clarification. It turns out that just updating the "mixed" value for single font weights is impossible because we can have a variety of ways the text element is "selected" or in focus so it's hard to tell if we have 1 font weight or some inline weights set. Given that, I combined your focused directives with feedback from Morten and have the following:
I believe this takes care of the bug and could allow future edge cases surrounding the specifics to inline styles and the faux selection process that can sometimes inhibit the above updates occurring to be handled on a granular level with the attention it'll deserve since that will necessitate updates to more functionality.
Thoughts?
That sounds good, @BrittanyIRL, thanks much!
Verified in QA
Verified in QA
Most helpful comment
The original issue, what this ticket is trying to solve, is the scenario in which:
We want to avoid that scenario. Since there is actually just one font weight being used, not multiple, we shouldn't show multiple because the font wasn't resetting to a valid, available font weight.
In the GIF you added https://github.com/google/web-stories-wp/pull/4734#issuecomment-704347155 I see that the base case is that you do have multiple font weights in the font element. In that case, when you change the font family if you still have multiple font weights it's perfectly fine to have "multiple" in the font weight drop down, both before and after changing the font family. Ideally, we'd want font weights to be one of the valid font weights for that font family, but I don't think that's required for this bug.
So, to clarify acceptance criteria for this bug;