Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

mixed charset regression #444

Closed
coolo opened this Issue Jan 4, 2013 · 11 comments

Comments

Projects
None yet
4 participants

coolo commented Jan 4, 2013

I have a test suite that worked with bca84f9 (last commit last year) but fails with master

<"Invalid package name: 'Testing包صفقة'"> expected but was
<"Invalid package name: 'Testing包">.

The HTML queried is this:

Invalid package name: 'Testing包صفقة'

and first(:css, "div#flash-messages p").text is "Testing包"

Before I fail again while reducing the test case, I better report it as I see it this time :)

coolo commented Jan 4, 2013

webkit_debug does not reveal anything unexpected:

Started "Node.text"
Finished "Node.text" with response "Success(Invalid package name: 'Testingå)"
Wrote response true "Invalid package name: 'Testingå"

coolo commented Jan 4, 2013

the reason for this regression is rather suprising to me, but this "fixes" the problem:

--- a/src/WebPage.cpp
+++ b/src/WebPage.cpp
@@ -70,7 +70,7 @@ void WebPage::loadJavascript() {
}

void WebPage::setUserStylesheet() {

  • QString data = QString("*, :first-line, :first-letter, :before, :after { font-family: 'Arial' ! important; }").toUtf8().toBase64();
  • QString data = QString("*, :first-line, :before, :after { font-family: 'Arial' ! important; }").toUtf8().toBase64();
    QUrl url = QUrl(QString("data:text/css;charset=utf-8;base64,") + data);
    settings()->setUserStyleSheetUrl(url);
    }

coolo commented Jan 4, 2013

reproducible in chromium - with that stylesheet .innerText is only the part without arabic. I changed my test case now not to include a line break :)

panozzaj commented Jan 8, 2013

It looks like changes related to this might have been introduced in #442

panozzaj added a commit to panozzaj/capybara-webkit that referenced this issue Jan 8, 2013

panozzaj commented Jan 8, 2013

I had the same issue with some tests of mine that worked fine locally, but when I pushed to tddium, I would get errors like: Expected to find "Apples" within text "pples ananas herries"

Where the text usually is "Apples Bananas Cherries". Basically it looked like the driver was disregarding the first and/or capitalized letters. I have a fork that has the change @coolo mentioned, as well as deleting the spec that it referenced. I'm not sure if I need to make a pull request. I couldn't get the specs running locally, but I think it should pass.

Collaborator

mhoran commented Jan 8, 2013

I've seen a few reports of issues with #442, however aside from this mixed character set regression (which is odd, but likely an issue with the underlying implementation of innerText and something that can be worked around), I've been unable to reproduce.

As reported in #435, it seems that :first_letter is causing segfaults on OS X, however this does not happen for me. I have not been able to come up with a test case that causes a segfault.

Similarly, I have not been able to come up with a test case that causes the first letter to disappear. If you could provide such a failing test case, that'd be great. I think it'd be best to revert #442, however without test cases there's nothing to prevent a regression.

Also, reverting #442 will cause segfaults for OS X users with web fonts. The only workaround for this is to upgrade to Qt 5, as the upstream Qt bug report has not gotten any attention.

Collaborator

mhoran commented Jan 13, 2013

I've released 0.14.1 to address this issue. We now only override web fonts for ::before and ::after pseudo elements and leave ::first-letter and ::first-line as is. This could cause segfaults for OS X users with web fonts applied to those pseudo elements, but that seems like less of an issue than the breakage reported here.

Collaborator

mhoran commented Feb 23, 2013

@coolo, has 0.14.1 resolved this issue for you?

coolo commented Feb 24, 2013

I changed my test case not to trigger the problem, so I don't know. I can check tomorrow (if I don't forget)

mikegee commented Sep 15, 2013

@coolo 7 months have passed. Can you confirm that this issue is resolved?

@mhoran mhoran closed this Oct 18, 2013

saschpe pushed a commit to saschpe/open-build-service that referenced this issue Nov 12, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment