Closed Bug 653495 Opened 11 years ago Closed 11 years ago

regexes in script mods and moz-document rules are inconsistent

Categories

(Add-on SDK Graveyard :: General, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: myk, Assigned: myk)

Details

Attachments

(1 file)

In SDK bug 627386, we landed support for regexes in match patterns <https://github.com/mozilla/addon-sdk/commit/45505a8c67a3b337715746b7340a6b578b8feb08> for page mods (i.e. script mods), and in CSS bug 398962, dbaron just landed support for them in -moz-document rules <https://hg.mozilla.org/mozilla-central/rev/52a05ef70c66>.

But the implementations are different, and since we plan to use -moz-document rules to implement style mods, they should be the same, so identical regexes can be used to specify the pages to which both script and style mods apply.

The restrictions on regexes in -moz-document rules are based on those in the HTML5 pattern attribute <http://dev.w3.org/html5/spec/common-input-element-attributes.html#attr-input-pattern>.  In particular:

* the pattern "must match the entire value, not just any subset(somewhat as if
  it implied a ^(?: at the start of the pattern and a )$ at the end)";

* the expression is compiled "with the global, ignoreCase, and multiline flags
  disabled".

At first glance, we should be able to apply these restrictions to the patterns the SDK accepts for script mods by validating the patterns against the rules and automatically enclosing them in ^(?: and )$.

The only restriction that feels like it might be painful is ignoreCase, although I think there's a way to write a regular expression for any URL whose case one wants to ignore.

I don't know whether we could instead loosen the restrictions in -moz-document rules.  Presumably they are partly about fitting patterns into single string values.

dbaron: can you speak to that?

If we decide to implement these restrictions in the SDK, we should at least document them in 1.0b5, even if we don't land the implementation by then, so we don't break the API later.
If we decide to fix this bug by implementing these restrictions in the SDK, then we should file a separate bug to first document them, and that bug will be P1 for 1.0b5.
Priority: -- → P2
Target Milestone: --- → 1.0
I didn't followed bug 398962 and I didn't realized that platform has already been changed to allow "CSS mod" in the SDK.

But it can be simplier if we keep document selection method in javascript, in the SDK source code. In order to do so, can't we have access to a method that would apply such stylesheet on one document? 
Like `userStylesheet.apply(document, css_url)`. 
Or Cascading forces us to apply/register user stylesheets only during page load ?
Wanting the regexp to match the whole value, and wanting it to be case-sensitive, were intentional on my part (URLs are, after all, case-sensitive).  And I'd rather introduce as few variations as possible into how regexps are used between different things in content/layout.
(In reply to comment #2)
> I didn't followed bug 398962 and I didn't realized that platform has already
> been changed to allow "CSS mod" in the SDK.
> 
> But it can be simplier if we keep document selection method in javascript, in
> the SDK source code. In order to do so, can't we have access to a method that
> would apply such stylesheet on one document? 
> Like `userStylesheet.apply(document, css_url)`. 
> Or Cascading forces us to apply/register user stylesheets only during page load
> ?

I believe you can apply (and remove) user stylesheets at any document load state, and the rules will take effect.  However, it seems even simpler to register the rules once, when the style mod is created, via nsIStyleSheetService::loadAndRegisterSheet, and let that service take care of whether and when to apply the style.


(In reply to comment #3)
> Wanting the regexp to match the whole value, and wanting it to be
> case-sensitive, were intentional on my part (URLs are, after all,
> case-sensitive).  And I'd rather introduce as few variations as possible into
> how regexps are used between different things in content/layout.

And I want to introduce no variation in how regexes are used between script and style mods.  Which suggests that we should implement these restrictions in script mods.
(In reply to comment #4)
> Which suggests that we should implement these restrictions in script mods.

Ok, that's what we'll do.  I have filed bug 653527 on the docs changes.
(In reply to comment #4)
> I believe you can apply (and remove) user stylesheets at any document load
> state, and the rules will take effect.  However, it seems even simpler to
> register the rules once, when the style mod is created, via
> nsIStyleSheetService::loadAndRegisterSheet, and let that service take care of
> whether and when to apply the style.
> 

May be simplier, be way more limiting. With such pratice we won't be able to attach CSS "on demand", for example, during:
  tab.attach({stylesheetFile : "..."});

This global pattern of "page mod on-demand" is very important and represent a lot of usecases and I think it is going to be the same with CSS. Some use cases is going to require to apply a stylesheet to one specific tab on a particular user action.

Again with something like nsIStyleSheetService::applySheetToDocument(doc, sheetUrl) we would be able to have the same registry to apply JS and CSS (I found it simplier) AND we would be able to apply CSS dynamically on arbitrary documents.

But as I said, I don't really know cascade limitations, so it may be impossible or overcomplicated to have such feature in platform and I could understand that we are working around it.
(In reply to comment #6)
> May be simplier, be way more limiting. With such pratice we won't be able to
> attach CSS "on demand", for example, during:
>   tab.attach({stylesheetFile : "..."});
>
> This global pattern of "page mod on-demand" is very important and represent a
> lot of usecases and I think it is going to be the same with CSS. Some use cases
> is going to require to apply a stylesheet to one specific tab on a particular
> user action.
> 
> Again with something like nsIStyleSheetService::applySheetToDocument(doc,
> sheetUrl) we would be able to have the same registry to apply JS and CSS (I
> found it simplier) AND we would be able to apply CSS dynamically on arbitrary
> documents.

Yup, you're right, we'll need the ability to apply CSS to a document on demand.  That seems like an orthogonal issue, however.  Even with that feature, it would still make sense to apply CSS to documents as they load via the existing mechanism.

Can you file a platform bug on adding nsIStyleSheetService::applySheetToDocument?

In the meantime, here's a patch that applies the same restrictions to SDK match patterns as exist for -moz-document rules.

Note that the patch includes no documentation changes; that's because it turns out there is no documentation on this new feature, so there's nothing to change.  Once we do document it, however, we should be sure to describe the restrictions.

Also note that this patch isn't essential for 1.0b5, given that the feature isn't documented.
Assignee: nobody → myk
Status: NEW → ASSIGNED
Attachment #529239 - Flags: review?(warner-bugzilla)
Comment on attachment 529239 [details] [diff] [review]
patch v1: implements restrictions

Looks good to me. It'd be easier to read if all of the "ok()" calls in testMatchPatternTestFalse() used a different function name (maybe "no()"?), since just looking at the diff, it's hard to tell that e.g. ok(/zilla.*/, URL) is actually asserting that it *doesn't* match. Maybe combine both testMatchPatternTestTrue and testMatchPatternTestFalse into a single call, define both yes() and no() functions like the old ok() function, then interleave the tests as necessary to point out how /.*zilla.*/ is ok bit /zilla.*/ is not.

But that's test cleanup, and could be deferred to a different bug. The patch as it stands is fine.
Attachment #529239 - Flags: review?(warner-bugzilla) → review+
(In reply to comment #8)
> Looks good to me. It'd be easier to read if all of the "ok()" calls in
> testMatchPatternTestFalse() used a different function name (maybe "no()"?),
> since just looking at the diff, it's hard to tell that e.g. ok(/zilla.*/, URL)
> is actually asserting that it *doesn't* match. Maybe combine both
> testMatchPatternTestTrue and testMatchPatternTestFalse into a single call,
> define both yes() and no() functions like the old ok() function, then
> interleave the tests as necessary to point out how /.*zilla.*/ is ok bit
> /zilla.*/ is not.

I agree, and that's the way dbaron's -moz-document patch is written.  Let's remember to make this larger change when we get a chance.

Although this patch isn't essential for 1.0b5, it's useful for it, since it means users will never be exposed to unrestricted patterns, so their 1.0b5-based addons won't break.  And it's relatively low-risk.  So I've granted it approval to land and have landed it.

https://github.com/mozilla/addon-sdk/commit/5caede27927947184cd691b84643d4a253f7df5c
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.