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

Rich Text: Set applied format as active in applyFormats #15573

Merged
merged 7 commits into from May 13, 2019

Conversation

@aduth
Copy link
Member

commented May 10, 2019

Fixes #15503
Fixes #15522

This pull request seeks to resolve an issue where updates to links (and presumably any applied format?) may not immediately appear as having taken effect in the UI.

Implementation notes:

I'm not entirely comfortable in this realm of the code, but judging by some existing logic which modifies active formats in applyFormats for formats applied to collapsed text where the format was not already applied, I thought it might be safe to extrapolate this out to include the format as being considered active for any provided value. Alternative suggestions welcome.

Testing instructions:

Repeat Steps to Reproduce from #15503 and #15522, verifying expected results.

Ensure unit tests pass:

npm run test-unit packages/rich-text
// Always revise active formats. This serves as a placeholder for new
// inputs with the format so new input appears with the format applied,
// and ensures a format of the same type uses the latest values.
activeFormats: [ format, ...activeFormats ],

This comment has been minimized.

Copy link
@ellatrix

ellatrix May 10, 2019

Member

I wonder if order could matter here. Could be not just copy the formats at the selection start? Or perhaps use getActiveFormats?

This comment has been minimized.

Copy link
@aduth

aduth May 10, 2019

Author Member

In my testing, it did matter: Where due to the behavior of normaliseFormats, the 'latest' format must occur first, since normaliseFormats defers to the previous entry:

if ( isFormatEqual( format, previousFormat ) ) {
newFormatsAtIndex[ formatIndex ] = previousFormat;
}

I suppose I ought to document this more clearly.

This comment has been minimized.

Copy link
@aduth

aduth May 10, 2019

Author Member

Although looking closer at isFormatEqual, it's odd that it would have made any effective difference for me... considering that "latest" shouldn't matter if the attribute values (i.e. target="_blank" was not the same between formats anyways).

@aduth

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

Although looking closer at isFormatEqual, it's odd that it would have made any effective difference for me... considering that "latest" shouldn't matter if the attribute values (i.e. target="_blank" was not the same between formats anyways).

I was looking at this wrong: activeFormats isn't considered for normaliseFormats. What I was seeing was explained by the fact that the active format was duplicated, and order mattered for how it was chosen for the UI. The changes in c8c5ed4 avoid duplication by ensuring that an equivalent format by type is first rejected.

I also found that removeFormats ought to be updated as well. This change was included in c4ebe80.

Generally I'm still a bit wary about how this all works, mostly because "active formats" seems like a UI concern which shouldn't be considered in this module. But given the precedent, this seems an improvement all the same.

Could be not just copy the formats at the selection start? Or perhaps use getActiveFormats?

I tried using getActiveFormats but it was problematic both because (a) it only considered formats at the start (not expected?) and (b) some difficulty around implementing the "placeholder" effect (I don't recall specifically if it was something of my own doing). Generating the activeFormats argument for this is essentially doing all the work already. In the end, I'm not really sure what purpose this function serves.

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

Anyone tested if this affects WP 5.2?

@ellatrix

This comment has been minimized.

Copy link
Member

commented May 11, 2019

I'll have a closer look Monday.

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

In my testing, this is looking good.

@aduth aduth requested review from ajitbohra, gziolo, nerrad, ntwb and talldan as code owners May 13, 2019

@aduth

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

d1843ee includes an updated end-to-end test which should verify regression fix against both #15503 and #15522 .

In scanning for other references to active formats within the rich-text module, I observe there are native equivalents to these files:

The changes in this pull request shouldn't introduce conflicts, though it is likely that if the logic was meant to align, the revisions here would not be reflected. I don't personally understand well enough why these files exist as native-specific to judge whether changes to those files are necessary (cc @koke , @hypest ).

@aduth

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

I observe there is a utility function updateFormats which isn't used internal to the rich-text module itself, but could potentially prove a useful abstraction to normalizing how active formats are updated:

https://github.com/WordPress/gutenberg/blob/master/packages/rich-text/src/update-formats.js

This seems more as a general refactoring / consolidation point, however.

@koke

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

The changes in this pull request shouldn't introduce conflicts, though it is likely that if the logic was meant to align, the revisions here would not be reflected. I don't personally understand well enough why these files exist as native-specific to judge whether changes to those files are necessary (

@Tug I don't remember why these were introduced, can you comment here?

@Tug

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

We added those because we didn't want any regression in the editor compared to aztec (the mobile editor), especially regarding writing in multiple formats at once.

At the time gutenberg was not able to handle that properly so we "forked" the rich-text lib and hacked the formatPlaceholder prop to store the "active formats" information.

This is being removed in our fix PR which tries to bring in all the changes from #14640 into mobile.

@aduth

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

Thanks @koke , @Tug . Since I'd not expect these changes to conflict with that effort, I'll operate on the assumption that I don't need to make any revisions to account for these files.

@aduth aduth merged commit 449c254 into master May 13, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@aduth aduth deleted the fix/15503-rich-text-applied-format branch May 13, 2019

@youknowriad youknowriad added this to the 5.7 (Gutenberg) milestone May 13, 2019

youknowriad added a commit that referenced this pull request May 15, 2019

Rich Text: Set applied format as active in applyFormats (#15573)
* Rich Text: Set applied format as active in applyFormats

* Rich Text: Update toggleFormat tests per activeFormats revision

* Rich Text: Replace existing active format in applied

* Rich Text: Remove active format in all cases for removeFormat

* Rich Text: Update toggleFormat tests per removeFormats revision

* Testing: Update E2E test case to verify link display regression

* Rich Text: Verify by test of activeFormats format replacement

youknowriad added a commit that referenced this pull request May 16, 2019

Rich Text: Set applied format as active in applyFormats (#15573)
* Rich Text: Set applied format as active in applyFormats

* Rich Text: Update toggleFormat tests per activeFormats revision

* Rich Text: Replace existing active format in applied

* Rich Text: Remove active format in all cases for removeFormat

* Rich Text: Update toggleFormat tests per removeFormats revision

* Testing: Update E2E test case to verify link display regression

* Rich Text: Verify by test of activeFormats format replacement
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.