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

Use span instead of div in the text layer #10197

Merged
merged 2 commits into from
Nov 18, 2018

Conversation

PalmerAL
Copy link
Contributor

@PalmerAL PalmerAL commented Oct 30, 2018

This improves copy/pasting text content since it reduces the amount of unnecessary newlines.

@timvandermeij
Copy link
Contributor

/botio test

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://54.215.176.217:8877/e2ac2c1d78b0882/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://54.67.70.0:8877/3b9b3895a21e26f/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Failed

Full output at http://54.67.70.0:8877/3b9b3895a21e26f/output.txt

Total script time: 19.21 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: FAILED

Image differences available at: http://54.67.70.0:8877/3b9b3895a21e26f/reftest-analyzer.html#web=eq.log

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

Full output at http://54.215.176.217:8877/e2ac2c1d78b0882/output.txt

Total script time: 25.07 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: FAILED

Image differences available at: http://54.215.176.217:8877/e2ac2c1d78b0882/reftest-analyzer.html#web=eq.log

@PalmerAL
Copy link
Contributor Author

It looks like the CSS for the <span> elements isn't being applied for some reason, so they aren't positioned correctly. I tested a couple of the PDF's that are failing, and they all seem to work fine locally, so I can't think of any reason why they would be failing in the tests. Is there something I'm missing?

@timvandermeij
Copy link
Contributor

Ah, I think you also need to change https://github.com/mozilla/pdf.js/blob/master/test/text_layer_test.css#L25 for the tests.

@PalmerAL
Copy link
Contributor Author

Thanks for pointing that out! It looks like there's some other CSS rules that depend on the items being divs as well, I think I've fixed them all in 45279d2.

@timvandermeij
Copy link
Contributor

/botio test

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

Command cmd_test from @timvandermeij received. Current queue size: 1

Live output at: http://54.215.176.217:8877/779528733d58856/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://54.67.70.0:8877/219385726c29ce4/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/219385726c29ce4/output.txt

Total script time: 19.11 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: Passed

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

Full output at http://54.215.176.217:8877/779528733d58856/output.txt

Total script time: 24.56 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: Passed

@timvandermeij
Copy link
Contributor

/botio-linux preview

@pdfjsbot
Copy link

pdfjsbot commented Nov 1, 2018

From: Bot.io (Linux m4)


Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://54.67.70.0:8877/7daa5ccb06f81eb/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Nov 1, 2018

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/7daa5ccb06f81eb/output.txt

Total script time: 2.84 mins

Published

@timvandermeij timvandermeij merged commit 5f15dc2 into mozilla:master Nov 18, 2018
@timvandermeij
Copy link
Contributor

timvandermeij commented Nov 18, 2018

Thank you for your contribution; I think this is a good improvement! I copy/pasted the following paragraph of the Tracemonkey paper with and without your patch applied. Before applying your patch the result was:

A
typed trace
is a trace annotated with a type for every variable
(including temporaries) on the trace. A typed trace also has an entry
type map
giving the required types for variables used on the trace
before they are defined. For example, a trace could have a type map
(x: int, b: boolean)
, meaning that the trace may be entered
only if the value of the variable
x
is of type
int
and the value of
b
is of type
boolean
. The entry type map is much like the signature
of a function.

In contrast, after applying your patch the result was:

Atyped traceis a trace annotated with a type for every variable(including temporaries) on the trace. A typed trace also has an entrytype mapgiving the required types for variables used on the tracebefore they are defined. For example, a trace could have a type map(x: int, b: boolean), meaning that the trace may be enteredonly if the value of the variablexis of typeintand the value ofbis of typeboolean. The entry type map is much like the signatureof a function.

Obviously there is enough room for improvement, but at least the unnecessary newlines are now gone.

Unrelated note: this is also the first time I used GitHub's squash-and-commit feature and it worked really well since it also allows you to choose the new commit message for the squashed commits.

@timvandermeij timvandermeij changed the title use <span> instead of <div> in textLayer Use span instead of div in the text layer Nov 18, 2018
@PalmerAL
Copy link
Contributor Author

@timvandermeij Thanks for merging this! Any thoughs on what to do with #10172? If you think the complexity of that isn't worth it (which I completely understand), one alternative I've been thinking about is whether it would be possible to unconditionally insert a space before every text layer item, which would fix things like your example above; if that sounds OK to you, I can spend some time investigating that further.

@PalmerAL PalmerAL deleted the textlayer-span branch November 19, 2018 04:41
@timvandermeij
Copy link
Contributor

@PalmerAL Yes, I think that one is still a bit too complex, at least in its current form. Perhaps we can do it in smaller steps, i.e., first try to get the spaces right and after that perhaps the newlines. If you can find a solution where the spaces are correct and no double ones are inserted, feel free to create a PR for that and we'll review. I'll close the current PR for you. Thank you for looking into this; if we take small steps here I'm sure we can make more improvements.

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

Successfully merging this pull request may close these issues.

3 participants