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

Support for named strings in margin-boxes #246

Merged
merged 8 commits into from Apr 30, 2015

Conversation

3 participants
@E-M-P-I-R-E

E-M-P-I-R-E commented Mar 20, 2015

Named strings using the -weasy-string-set style and the CSS string function passed to content().
http://dev.w3.org/csswg/css-gcpm/#named-strings
named_strings

More discussion in feature request - issue #174

Below is a simple example (more in tests/test_boxes.py).

<html>
<head>
<style>
body{
     counter-reset: header_counter; 
}
@page:first{
    @top-center { content: string(header)}
}
@page{
    @top-center { content: string(header, first-except)}
}
.page{
      page-break-before: always;
}

h1{
    counter-increment: header_counter;
    string-set: header content(after);
    -weasy-string-set: header content(text);
}
h1:before{
    content: "Chapter " counter(header_counter) " ";
}
</style>
</head>
<body>
    <div class="page"><h1>First</h1>This demonstrates named strings using the `string-set` style and the CSS `string` function passed to `content()`.<br>There are two assignments "First" and "Second" plus the psuedo ::before element. The numbers are produced using CSS `counter`.</div>
    <div class="page">CONTENT</div>
    <div class="page" ><h1 >Second</h1>Pages after the first have `first-except` set, meaning the page on which the assignment is made is excluded. Proceeding pages will have the value set here ("Second").</div>
    <div class="page">CONTENT</div>
</body>
</html>

Mike Z. Salem added some commits Mar 9, 2015

Mike Z. Salem Mike Z. Salem
Bug fix for string-set validation not accepting no argument (default …
…should be 'text')

Alter TEXT_CONTENT_EXTRACTORS so that string-set can use ::before/::after
 - Don't allow empty string to be assigned to string set (happends with psuedo elements)
Added some documentation to explain string function resolution
Mike Z. Salem Mike Z. Salem
Remove support of string set using psuedo elements. Was unsure of the…
… change to the function in build.TEXT_CONTENT_EXTRACTORS. That would be part of the text anyways.

Combined test functions to conform to coding style of other tests.
@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Mar 20, 2015

Member

This looks great! I’ll comment on specifics inline.

Tests fail on Travis-CI because:

  • flake8 complains about whitespace and overlong lines
  • xrange doesn’t exist on Python 3.x. Add from ..compat import xrange at the top of the file.
Member

SimonSapin commented Mar 20, 2015

This looks great! I’ll comment on specifics inline.

Tests fail on Travis-CI because:

  • flake8 complains about whitespace and overlong lines
  • xrange doesn’t exist on Python 3.x. Add from ..compat import xrange at the top of the file.
@@ -1267,6 +1269,23 @@ def bookmark_level(token):
return 'none'
@validator(prefixed=True) # CSS3 GCPM, experimental
def string_set(tokens):

This comment has been minimized.

@SimonSapin

SimonSapin Mar 20, 2015

Member

I have a hard time following this, sorry. Could you clarify what set of syntax you mean to support? All unsupported syntax should be rejected.

@SimonSapin

SimonSapin Mar 20, 2015

Member

I have a hard time following this, sorry. Could you clarify what set of syntax you mean to support? All unsupported syntax should be rejected.

"""
last = keyword == "last"
if self.current_page in self.string_set[name]:

This comment has been minimized.

@SimonSapin

SimonSapin Mar 20, 2015

Member

Does this crash if name is not in self.string_set? Maybe replace all occurrences of self.string_set[name] with a single assignments = self.string_set.get(name) and check for assignments is None.

@SimonSapin

SimonSapin Mar 20, 2015

Member

Does this crash if name is not in self.string_set? Maybe replace all occurrences of self.string_set[name] with a single assignments = self.string_set.get(name) and check for assignments is None.

This comment has been minimized.

@E-M-P-I-R-E

E-M-P-I-R-E Mar 21, 2015

It doesn't crash. The way the defaultdict is structured and evaluated [when resolving the value] is checking the current page for assignments, then previous pages... finally returning an empty string if there were no assignments.

@E-M-P-I-R-E

E-M-P-I-R-E Mar 21, 2015

It doesn't crash. The way the defaultdict is structured and evaluated [when resolving the value] is checking the current page for assignments, then previous pages... finally returning an empty string if there were no assignments.

This comment has been minimized.

@SimonSapin

SimonSapin Mar 21, 2015

Member

Oh, right, I forgot it was a defaultdict.

@SimonSapin

SimonSapin Mar 21, 2015

Member

Oh, right, I forgot it was a defaultdict.

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Mar 20, 2015

Member

Alright, I’m done reviewing up to 7db9be4. There are some minor issues, but the overall design is simpler than I expected. Good job!

Member

SimonSapin commented Mar 20, 2015

Alright, I’m done reviewing up to 7db9be4. There are some minor issues, but the overall design is simpler than I expected. Good job!

Mike Z. Salem Mike Z. Salem
Set `string-set` default to 'none', altered validation & resolution […
…do nothing in that case]

Added validation to the second argument to `string()`
More info./discussion here: #246 (comment)
@E-M-P-I-R-E

This comment has been minimized.

Show comment
Hide comment
@E-M-P-I-R-E

E-M-P-I-R-E Mar 21, 2015

Oops, I guess I should have responded to some of your comments before pushing again [I was planning on it]. I think I addressed everything in this latest commit. Mostly, changed the validation.

One oddity - you suggested a change in TEXT_CONTENT_EXTRACTORS['box_text_contents']. I wasn't actually using that function (meant to remove that code), so I made that change to the original function in formatting_structure/build. I also added a new key text which calls that function. In short, I'm hoping it's okay to have made that change to the function in build (which is also used to resolve bookmark-labels, at least).

Oh.. I just figured out how to see the specific issues that are causing continuous integration to fail. I'll take care of that.

Two things that I'm unclear about
First - from the spec.:

User agents must be able to recall many values of the named string, as the string() function can return past, current, or future values of the assignment.

Second: should there be a warning when attempting to use named strings outside of the margin-boxes?

Thanks a lot!

E-M-P-I-R-E commented Mar 21, 2015

Oops, I guess I should have responded to some of your comments before pushing again [I was planning on it]. I think I addressed everything in this latest commit. Mostly, changed the validation.

One oddity - you suggested a change in TEXT_CONTENT_EXTRACTORS['box_text_contents']. I wasn't actually using that function (meant to remove that code), so I made that change to the original function in formatting_structure/build. I also added a new key text which calls that function. In short, I'm hoping it's okay to have made that change to the function in build (which is also used to resolve bookmark-labels, at least).

Oh.. I just figured out how to see the specific issues that are causing continuous integration to fail. I'll take care of that.

Two things that I'm unclear about
First - from the spec.:

User agents must be able to recall many values of the named string, as the string() function can return past, current, or future values of the assignment.

Second: should there be a warning when attempting to use named strings outside of the margin-boxes?

Thanks a lot!

@E-M-P-I-R-E

This comment has been minimized.

Show comment
Hide comment
@E-M-P-I-R-E

E-M-P-I-R-E Mar 21, 2015

Question was raised:

Do the 'keyword' and 'name' strings add information here, or could the return value be just (keyword, name)? (Or (name, keyword), to match the order of the syntax?)

I was following the example of the validation function for bookmark-label [which returns ('string', token.value)] but I liked the suggestion and went with that.

string-set validation now returns (var_name, keyword). It's only accessed in layout/pages.make_page() (when updating the LayoutContext with new assignments for a given named-string).

E-M-P-I-R-E commented Mar 21, 2015

Question was raised:

Do the 'keyword' and 'name' strings add information here, or could the return value be just (keyword, name)? (Or (name, keyword), to match the order of the syntax?)

I was following the example of the validation function for bookmark-label [which returns ('string', token.value)] but I liked the suggestion and went with that.

string-set validation now returns (var_name, keyword). It's only accessed in layout/pages.make_page() (when updating the LayoutContext with new assignments for a given named-string).

Mike Z. Salem Mike Z. Salem
More changes for named string support
Reinstate support for psuedo elements as an arugument to `content()`
Have to check if a box is a `ParentBox` when using `decendants()` in `TEXT_CONTENT_EXTRACTORS` for psuedo elements because it was returning the value twice in a row (ex. "stringstring")
@E-M-P-I-R-E

This comment has been minimized.

Show comment
Hide comment
@E-M-P-I-R-E

E-M-P-I-R-E Mar 21, 2015

It was also requested that I explain what I intended to support for string-set (in terms of validation).

The answer is one value pair ex:
-weasy-string-set: header content();

If no argument is passed to content() it defaults to 'text'.

Arg, I apologize for hitting close accidentally.

I just re-instated support for 'before' and 'after' as arguments to content() when passed to string-set.

E-M-P-I-R-E commented Mar 21, 2015

It was also requested that I explain what I intended to support for string-set (in terms of validation).

The answer is one value pair ex:
-weasy-string-set: header content();

If no argument is passed to content() it defaults to 'text'.

Arg, I apologize for hitting close accidentally.

I just re-instated support for 'before' and 'after' as arguments to content() when passed to string-set.

@E-M-P-I-R-E E-M-P-I-R-E reopened this Mar 21, 2015

Mike Z. Salem added some commits Mar 21, 2015

Mike Z. Salem Mike Z. Salem
More changes for named string support
Pep8/flake8 conformance
Mike Z. Salem Mike Z. Salem
More changes for named strings [3]
PEP8/flake8 compliance again (sorry)

@liZe liZe self-assigned this Apr 29, 2015

@liZe liZe added the feature label Apr 29, 2015

@liZe

This comment has been minimized.

Show comment
Hide comment
@liZe

liZe Apr 29, 2015

Member

Wow. So cool!

I'll review that and merge it soon. Thanks @E-M-P-I-R-E for your work, and @SimonSapin for your review :).

Member

liZe commented Apr 29, 2015

Wow. So cool!

I'll review that and merge it soon. Thanks @E-M-P-I-R-E for your work, and @SimonSapin for your review :).

liZe added a commit that referenced this pull request Apr 30, 2015

Merge pull request #246 from E-M-P-I-R-E/master
Support for named strings in margin-boxes

@liZe liZe merged commit 790af14 into Kozea:master Apr 30, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment