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

T/16957 [PFW] Styles.inliner.inline does not take selectors specificity into account and duplicates rules. #348

Merged
merged 7 commits into from Apr 25, 2017

Conversation

Projects
None yet
3 participants
@f1ames
Copy link
Member

f1ames commented Apr 18, 2017

The fix keeps the order of the css rules when parsing. The rules containing class selectors have higher priority and are applied first.

The styles already existing on the element are not overwritten which means:

  • inline styles are preserved
  • styles with the highest priority are applied first

The second point above means that the styles inlining applies rules containing class selectors first in a reversed order (so the last on goes first) and then rest of the rules also in reversed order.

Please keep in mind that this solution is a minimalistic (almost) approach to solve this specific issue. There are some cases which are not covered (more complex selectors, rules with multiple selectors, etc), however as Excel or Word does not generate such styles it is sufficient.

@Comandeer Comandeer self-requested a review Apr 20, 2017

@Comandeer
Copy link
Member

Comandeer left a comment

LGTM. However there is one question: is it likely that Word produce such markup

li.MsoNormal {
   background-color: #000;
   color: #555;
}
.MsoNormal {
   background-color: #FFF;
}

If it's possible, then the current solution should be updated.

cc @mlewand

@mlewand mlewand self-requested a review Apr 20, 2017

@f1ames

This comment has been minimized.

Copy link
Member Author

f1ames commented Apr 21, 2017

Just would like to point out that styles mentioned by @Comandeer in the previous comment are used in test to check if order of the selectors with the same priority (ofc in this implementation) is preserved when parsing and inlining styles. cc @mlewand

@mlewand

This comment has been minimized.

Copy link
Member

mlewand commented Apr 21, 2017

As for specificity we need only a minimal viable product here. So as long as current implementation solves the problem, this is all we need.

I checked a fancy/complex word file with multiple and haven't faced case like this. In case if some version will ever produce any incompatible markup we'll simply fix it in a patch release.

Structure

I find proposed structure redundant. I feel that it might be simplified by turning returned result into an array, and change a strucutre of entries.

So rather than:

.MsoChpDefault
	{mso-style-type:export-only;
	mso-default-props:yes;
	font-family:Calibri;}
p.MsoNormal, li.MsoNormal, div.MsoNormal {
	mso-style-unhide:no;
	mso-style-qformat:yes;
	mso-style-parent:"";
	margin:0cm;
	margin-bottom:.0001pt;
}
=>
{
	"styles": {
		".MsoChpDefault": {
			"font-family": "Calibri"
		},
		"p.MsoNormal, li.MsoNormal, div.MsoNormal": {
			"margin": "0cm 0cm 0.0001pt"
		}
	},
	"order": [ ".MsoChpDefault", "p.MsoNormal, li.MsoNormal, div.MsoNormal" ]
}

I'd propose:

.MsoChpDefault
	{mso-style-type:export-only;
	mso-default-props:yes;
	font-family:Calibri;}
p.MsoNormal, li.MsoNormal, div.MsoNormal {
	mso-style-unhide:no;
	mso-style-qformat:yes;
	mso-style-parent:"";
	margin:0cm;
	margin-bottom:.0001pt;
}
=>
[
	{
		"selector": ".MsoChpDefault",
		"styles": {
			"font-family": "Calibri"
		}
	},
	{
		"selector": "p.MsoNormal, li.MsoNormal, div.MsoNormal",
		"styles": {
			"margin": "0cm 0cm 0.0001pt"
		}
	}
]

Does it look feel better? Please drop a 👍👎

@Comandeer

This comment has been minimized.

Copy link
Member

Comandeer commented Apr 21, 2017

It looks better but isn't it going to complicate the logic for determining correct order of declarations?

@mlewand

This comment has been minimized.

Copy link
Member

mlewand commented Apr 21, 2017

I think it's worth the effort, also please extract/encapsulate sorting function into a separate function, so that parseStyleTags doesn't get super messy.

@mlewand mlewand removed their request for review Apr 21, 2017

@f1ames

This comment has been minimized.

Copy link
Member Author

f1ames commented Apr 24, 2017

Applied changes proposed by @mlewand. The code looks much nicer and I was able to reduce its size a little :)

@f1ames f1ames requested review from Comandeer and mlewand Apr 24, 2017

@mlewand

This comment has been minimized.

Copy link
Member

mlewand commented Apr 24, 2017

@Comandeer, please handle the review, thanks!

@mlewand mlewand removed their request for review Apr 24, 2017

return styles;
}

function getCompareFunction( styles ) {

This comment has been minimized.

Copy link
@Comandeer

Comandeer Apr 24, 2017

Member

I'd move whole sorting logic into dedicated function, CKEDITOR.plugins.pastefromword.styles.inliner.sort.

@f1ames f1ames requested a review from Comandeer Apr 25, 2017

f1ames added some commits Apr 18, 2017

@f1ames

This comment has been minimized.

Copy link
Member Author

f1ames commented Apr 25, 2017

Rebased on latest major, one fixture needed some adjustments - 59c3dc6.

@f1ames f1ames requested a review from Comandeer Apr 25, 2017

@Comandeer Comandeer merged commit dec0800 into ckeditor:major Apr 25, 2017

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@Comandeer Comandeer deleted the cksource:t/16957 branch Apr 25, 2017

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.