Neerja Pancholi[:neerja] Assignee
	Description • 8 years ago

The fix for this will fix three reftests for multicol related properties:

Fails on Firefox, passes on Chrome->
1. multicol-nested-column-rule-001.xht
2. multicol-rule-samelength-001.xht

Fails on Firefox, fails on Chrome
3. multicol-columns-invalid-002.xht

	David Baron :dbaron:
	Comment 1 • 8 years ago

Perhaps this should be fixed as part of the bug that introduced the problem rather than creating a separate bug?

	Neerja Pancholi[:neerja] Assignee
	Comment 2 • 8 years ago

(In reply to Neerja:neerja from comment #0)
> The fix for this will fix three reftests for multicol related properties:
> 
> Fails on Firefox, passes on Chrome->
> 1. multicol-nested-column-rule-001.xht
> 2. multicol-rule-samelength-001.xht
> 
> Fails on Firefox, fails on Chrome->
> 3. multicol-columns-invalid-002.xht

Adding to original description:
The tests mentioned above all fail because of some injections made by import-test.py, EG:

Both multicol-nested-column-rule-001.xht and multicol-columns-invalid-002.xht have invalid string '--moz' being inserted which leads to a parsing failure. 

multicol-rule-samelength-001.xht has injections like below that need to be fixed:
  -moz--moz-column-rule-color: green;
  -moz--moz-column-rule-style: solid;
  -moz--moz-column-rule-width: 5em;

	Neerja Pancholi[:neerja] Assignee
	Comment 3 • 8 years ago

(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #1)
> Perhaps this should be fixed as part of the bug that introduced the problem
> rather than creating a separate bug?

Sorry about the ambiguous bug title and description before. This is a completely new bug, added more details to the bug description and changed the title to make it clear.

	Neerja Pancholi[:neerja] Assignee
	Comment 4 • 8 years ago

[sorry for me & dholbert editing the summary so many times; I think we've got the scope defined now  :)]
To further-clarify the goal here: right now, import-tests.py breaks some multicol tests when it imports them, so that they end up with XML parse errors from having "--" inside of an attribute in some cases, and bogus "-moz--moz-column-*" CSS in other cases.  In this bug, I intend to fix import-tests.py so that it doesn't introduce those problems.  Then after that's fixed, we can import the CSSWG multicol reftests (without introducing these problems!) in Bug 1295261

	David Baron :dbaron:
	Comment 7 • 8 years ago
mozreview-review

Comment on attachment 8787013 [details]
Bug 1299012 - Fix import-tests.py to avoid breaking three multicol reftests

https://reviewboard.mozilla.org/r/75846/#review76044

::: layout/reftests/w3c-css/import-tests.py:254
(Diff revision 2)
>              replacementLine = replacementLine.replace(rule, "-moz-" + rule)
> +            replacementLine = replacementLine.replace("--moz-", "-moz-")
> +            replacementLine = replacementLine.replace("-moz-moz-", "-moz-")

So instead of doing this, I think it would be better to change the first line so that the prefixed properties are only replaced when they're an entire word, rather than part of a word.  In particular, I think this would mean doing something more like just replacing the first line with:

replacementLine = re.sub("\\b" + rule + "\\b", "-moz-" + rule, replacementLine)


(I think it's also better to use something more like "for prop in aProps" rather than "for rule in aProps".)

If that works (which is partly judged by what changes in the imported tests it produces, if any), then r=dbaron with that.

	Neerja Pancholi[:neerja] Assignee
	Comment 9 • 8 years ago
mozreview-review-reply

Comment on attachment 8787013 [details]
Bug 1299012 - Fix import-tests.py to avoid breaking three multicol reftests

https://reviewboard.mozilla.org/r/75846/#review76044

> So instead of doing this, I think it would be better to change the first line so that the prefixed properties are only replaced when they're an entire word, rather than part of a word.  In particular, I think this would mean doing something more like just replacing the first line with:
> 
> replacementLine = re.sub("\\b" + rule + "\\b", "-moz-" + rule, replacementLine)
> 
> 
> (I think it's also better to use something more like "for prop in aProps" rather than "for rule in aProps".)

So I tried using the re.sub with \b but I found some issues with it. Because of this I had to make the following additions:

1. Added "if( re.match(".*href=.*", replacementLine) is None):" because '\b' was also matching cases when the href had dashes eg: href="path\to\file\multicol-columns-test.html" To prevent this line from being changed, I basically skipped all lines with href which happen to be all the lines with this meta data (did a manual check)

2. The other two lines i.e. ->
replacementLine = replacementLine.replace("--moz-", "-moz-")
replacementLine = replacementLine.replace("-moz-moz-", "-moz-")
had to be kept for cases when the same line matches more than one rule, leading to prefixes like -moz--moz- eg: Rule "column-rule" is a subset of rules "column-rule-color", "column-rule-style" and "column-rule-width". So, something like this would happen:
column-rule-color -> -moz-column-rule-color -> -moz--moz-column-rule-color

With these changes above, the treeherder build looks pretty good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3cf8c13d09ab

Let me know what you think. :)

	Neerja Pancholi[:neerja] Assignee
	Comment 12 • 8 years ago

Made some more changes related to the same fix. Even though this was already 'r+'ed by you, thought it would be a good idea to run the changes by you again. 
Comment #9 is no longer relevant.

	Daniel Holbert [:dholbert]
	Comment 13 • 8 years ago

Some additional context (from talking with Neerja):
 - MozReview doesn't seem to support re-requesting review when r+ has already been granted; hence, I suggested that Neerja co-opt the "needinfo" flag to get this back in your review queue (effectively).
 - The "\\b" that you suggested in comment 7 didn't end up helping, because python treats hyphens as a word-boundary.  (So all the false-positive matches were still matching.)  "[^-#]" seems to be sufficient to exclude all the places we want to exclude, though. (Excluding "-" lets us avoid matching href="multicol-columns-whatever.html" strings, and also avoid redundantly matching the same substring [which previously produced some "-moz-moz" issues].  And excluding "#" lets us avoid matching anchors in links to spec sections.)

	David Baron :dbaron:
	Comment 14 • 8 years ago
mozreview-review

Comment on attachment 8787013 [details]
Bug 1299012 - Fix import-tests.py to avoid breaking three multicol reftests

https://reviewboard.mozilla.org/r/75846/#review80110

This seems a little risky, since a CSS property could certainly come at the start of a line (e.g., in a style sheet that wasn't indented).

Could you add code that also does the substitution if it happens at the start of the line?

(Also, it would be good to see the diff to the imported tests at the same time as this fix.)

r=dbaron with that

	Neerja Pancholi[:neerja] Assignee
	Comment 15 • 8 years ago
mozreview-review-reply

Comment on attachment 8787013 [details]
Bug 1299012 - Fix import-tests.py to avoid breaking three multicol reftests

https://reviewboard.mozilla.org/r/75846/#review80110

I added the change to match start of line. Right now there is no test that matches this case but I tested this fix by editing a test locally and I see that it does get substituted correcty with this change. 
Also, since there was no change in the imported tests, I didn't repush those patches. 

But, all reimports of tests after applying any patch to this bug can be seen in the following patch:
Bug 1295261 - Reimport multicol tests after applying fix for import-tests.py (Bug 1299012)

*This patch is unchanged right now since no test matched the start of line case on rerunning import-tests.py

	David Baron :dbaron:
	Comment 17 • 8 years ago

Looks good.  Thanks.

	Pulsebot
	Comment 18 • 8 years ago

Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/409769321b11
Fix import-tests.py to avoid breaking three multicol reftests r=dbaron

	Carsten Book [:Tomcat]
	Comment 19 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/409769321b11