The link rel attribute should be handled as a token list to allow mixing manual link decorators for the same attribute
#17461
Conversation
…of attributes like class, style, rel or plain attribute.
|
@niegowski added |
|
Before merging, let's discuss the release strategy, if it's coupled or not with linking experience. |
There was a problem hiding this comment.
It's hard for me to approach this PR from top-level point of view and decide whether it is the best way to do it, but in overall, it makes sense and looks good. I rather focused on particular bits of code and possible logical/algorithmical/typo errors.
My two questions marks are:
- I am not sure if it was a good decision to move some of the code to view elements. I mean generating consumables and merging elements. I don't know what was the reason to move it (e.g. if multiple parts of the code wanted to perform the same logic and view element was the best to keep it). If you decide that this is a must have, I'd recommend taking care to see if all related elements are moved. In one place I commented that it is weird that view element imports a type from conversion. I had such feeling a few times. Right now I am not convinced that moving code to view elements was a necessary change, but I don't have the holistic view on this PR/improvement, you spent more time on it.
- Performance. Could you confirm that the performance does not drop, especially for samples where there is a lot of merging and unwrapping of view attribute elements (
formattingLongPs,inlineStyles,wiki,ghsfrom the recently introduced performance tests).
| public add( | ||
| element: Element, | ||
| consumables: Consumables | ||
| element: Text | Element | DocumentFragment, | ||
| consumables?: Consumables | NormalizedConsumables | ||
| ): void; |
There was a problem hiding this comment.
Do we need to keep this if this is the only way to use this method?
| @@ -250,7 +239,7 @@ export default class ViewConsumable { | |||
| * @param consumables.classes Class name or array of class names. | |||
| * @param consumables.styles Style name or array of style names. | |||
| */ | |||
| public revert( element: Node, consumables: Consumables ): void { | |||
| public revert( element: Node, consumables: Consumables | Match ): void { | |||
There was a problem hiding this comment.
Don't you want to add an example with Match as the second parameter to the API docs?
| } | ||
|
|
||
| if ( from.is( 'documentFragment' ) ) { | ||
| else if ( from.is( 'element' ) || from.is( 'documentFragment' ) ) { |
There was a problem hiding this comment.
BTW, we test from to be text, element and document fragment. What else it could be (there is no else branch, but I guess there's coverage for it)? Text proxy? Isn't this $text?
| for ( const child of ( from as Element | DocumentFragment ).getChildren() ) { | ||
| instance = ViewConsumable.createFrom( child, instance ); | ||
| for ( const child of from.getChildren() ) { | ||
| instance = ViewConsumable.createFrom( child, instance ); |
There was a problem hiding this comment.
The assignment here seems unnecessary and is confusing.
| /** | ||
| * Used by the {@link module:engine/view/matcher~Matcher Matcher} to collect matching attribute tuples | ||
| * (attribute name and optional token). | ||
| * |
There was a problem hiding this comment.
AFAICS, this is a replacement for the old matchPatterns function which was earlier in Matcher.
The old function had a very extensive API docs, while this one explains nothing about the method. I have no idea what this will return and how it will behave in different conditions. I can only read the source code, but during the review, I have no idea what is the expected outcome.
Please add more explanation and examples.
There was a problem hiding this comment.
Like, the description says that it collects something, but it returns boolean.
And, reading the code, it seems that it can populate the match array a bit, but then return false anyway. Am I supposed to always pass empty match array, or is it supposed to be used throughout several calls?But if so, then partial-population seems to be a bug.
I understand that we have to consider 1) old existing code that uses it; 2) performance; but it would be great to straighten this up. Also, I'd like to avoid passing match array to be populated, if possible. Can't we return null | Array?
| * @returns Returns `true` if elements are merged. | ||
| */ | ||
| public _mergeAttributesFrom( otherElement: Element ): boolean { | ||
| if ( !this._canMergeAttributesFrom( otherElement ) ) { |
There was a problem hiding this comment.
Following what I wrote in DowncastWriter. I think that _mergeAttributesFrom() should not make this check, instead, writer could do it. This will not result in extra performance overhead and will simplify code in writer.
| this._setAttribute( key, otherValue ); | ||
| } | ||
| else { | ||
| value._mergeFrom( otherValue ); |
There was a problem hiding this comment.
Not necessarily needs to fix it, but can't we use setAttribute with reset set to false? Isn't it the same? 🤔
| value instanceof StylesMap ? value._clone() : | ||
| new StylesMap( this.document.stylesProcessor ).setTo( String( value ) ) |
There was a problem hiding this comment.
These two lines look complicated, please simplify.
Co-authored-by: Szymon Cofalik <[email protected]>
Suggested merge commit message (convention)
Fix (engine): The link
relattribute will now allow mixing manual link decorators for the same attribute, as it will be now handled as a token list. Closes #13985, closes #6436.Fix (mention): Mention should not be wrapped with an additional
<span>when GHS is enabled. Closes #15329.Internal (html-support): Attribute matching updated to the new output of
Matcher#matchAll().MINOR BREAKING CHANGE (engine): The
ViewConsumable.consumablesFromElement()is removed and replaced withview.Element#_getConsumables()internal method. You should useViewConsumable.createFrom()To create consumables if needed.MINOR BREAKING CHANGE (engine): The
ViewElementConsumablesnow accepts and outputs only normalized data. TheViewConsumablestill accepts normalized or non-normalized input.MINOR BREAKING CHANGE (engine): The
Matcher#match()andMatcher#matchAll()output is now normalized. TheMatchResult#matchnow contains normalized data compatible with changes in theViewConsumable.Additional information
For example – encountered issues, assumptions you had to make, other affected tickets, etc.