ladybird
ladybird copied to clipboard
LadybirdBrowser
Wrong layout with `top`, `bottom`, `left`, `right` inside with `position: relative` and `position: absolute`
Summary
Thsi originally comes from https://browser.engineering/http.html noticing wrong layout for the "Hostname" and "Response Code" here:
and here:
Here's how they show up in Ladybird (at left, one on top of the other):
I managed to reduce to a simpler reduction below, see below for how it shows up in Chrome and in Ladybird
Operating system
Linux
Steps to reproduce
- Go to site (or try reduction)
- In Ladybird...
- ...And in Chrome
Expected behavior
As in Chrome, all properly layed out. Reduction in Chrome is:
Actual behavior
zoomed in for clarity
URL for a reduced test case
NN
HTML/SVG/etc. source for a reduced test case
<html>
<head>
<style>
.pre mark {
position: relative;
}
.pre label {
position: absolute;
}
.pre label.above {
top: -0.8em;
}
.pre label.below {
bottom: -1.6em;
}
.pre label.left {
left: 0;
}
.pre label.right {
right: 0;
}
</style>
<body>
<pre class='pre'>
<mark>BBBBBBBBBBBBBBB<label class='above left'>AAAAAAAAAAAAAAA<label class='below left'>CCCCCCCCCC</label></mark><mark>YYYYYYYYY<label class='above left'>XXXXXXXXX</label></mark>
</pre>
</body>
</html>
Log output and (if possible) backtrace
NN
Screenshots or screen recordings
See above
Build flags or config settings
Ladybird 3fcef0c519d60873f0c8ddeca0e1a3aa2857696c Linux (Ubuntu), Clang
Contribute a patch?
- [ ] I’ll contribute a patch for this myself.
Hey!
To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done?
Note that a pull request does not have to be "perfect" or "ready for merge" when you first open it. We just want it to be ready for a first review.
Cheers!
Carsonbot
this new store should probably in a bridge package, so that we can manage its dependency properly.
@stof Thank you for your feedback, I did move it to its own bridge, I'm not sure if I've done everything required from a structural POV.
I still didn't touch the tests yet
this new store should probably in a bridge package, so that we can manage its dependency properly.
While I agree on that @stof, right now there is no Bridge namespace:
@stof @OskarStark thank you for taking the time to review this 😄
I've actually updated the PR to add this Bridge namespace following the structure from the Messenger SQS example, but the package validation is failing and I'm unsure why as I've placed the composer.json file exactly in the same level as the Messenger SQS one, trying to investigate that right now
Yay! Package checks are passing, the match case for component_bridge wasn't behaving as expected, back onto PHP tests 😄
Yes, we need to keep in mind, that we need to configure a proper split for the repo etc.
@stof @OskarStark
The current state of this is:
- Moved the logic into its own component bridge
- Implemented integration tests by using localstack in CI to have a local DynamoDB instance
- Updated the default options logic based on feedback
- Implemented unit tests for the store constructor logic (DSN/Options)
I have re-requested your reviews, thank you 😄
Hi there! 👋 I'm sure everybody is quite busy prepping for 7.3 release, but just a gentle follow up on this one, I would like to know if anything else is needed from me so I can organise myself to have time to spend on this, thank you 😄
