1651530 - min-height on table is not honored

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[steve.patco]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Firefox/78.0

Steps to reproduce:

visit my site https://galzilla.com/index.php/blog/ in FF, the chrome and edge

Actual results:

Header image window is too small

Expected results:

Window for image should match image size

Edge and Chrome work correctly

Comment 1

[steve.patco]

Attachment: image.png

chrome to the right and FF to the left

Comment 2

[steve.patco]

Attachment: image.png

Just so you dont tell me that one is desktop and the other is android.

Comment 3

[steve.patco]

Attachment: image.png

Comment 4

[steve.patco]

Isnt anyone going to address this issue?

Comment 5

[Clara Guerrero ( Need Info Brindusa Tot Please)]

Hi Steve,

I'm unabe to enter the link provided.
I've tried chrome, ff beta 79.0b7 (64-bit) and ff nightly 80.0a1 (2020-07-13) (64-bit), I'm seeing the attached error.
Please confirm if you still need us to investigate in this issue, and check on your site so I can try to replicate on my end.

Thanks for the report.

Best regards, Clara.

Comment 6

[Clara Guerrero ( Need Info Brindusa Tot Please)]

Attachment: cannot load page.PNG

Comment 7

[steve.patco]

https://galzilla.com/index.php/blog/ works for me with no errors loading

Comment 8

[steve.patco]

I use CC allow in my firewall. You are probably outside the zone

Comment 9

[steve.patco]

??

Comment 10

[steve.patco]

This is incredible. Has firefox #hit the bed?

Comment 11

[Clara Guerrero ( Need Info Brindusa Tot Please)]

Hi Steve,

I was just able to access the link provided using connected to VPN USA; my apologies for the delay.
I was able to reproduce on windows 10 pro, on the following versions

Firefox Nightly version 80.0a1 (2020-07-14) (64-bit),
Beta 79.0b7
Release 78.0.2 (64-bit)

It doesn't happen in Chrome.
Setting a component for this issue in order to get the dev team involved.

Thanks for the report.
Best regards, Clara

Comment 12

[steve.patco]

(In reply to steve.patco from comment #0)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Firefox/78.0

Steps to reproduce:

visit my site https://galzilla.com/index.php/blog/ in FF, the chrome and edge

Actual results:

Header image window is too small

Expected results:

Window for image should match image size

Edge and Chrome work correctly

Be aware, CC_allow US CA MX only in CSF firewall

Comment 13

[Jessie [:jbonisteel] pls NI]

Potentially looks like more of a layout issue, so have moved components.

Comment 14

[steve.patco]

Interesting discovery. If I remove the slogan text from the header image, the image will completely disappear.

Comment 15

[steve.patco]

Well ALMOST disappear. No slogan will reduce image to a single line of visibility.

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Is there any chance that you can attach an HTML file that reproduces the issue to the bug, using the "attach file" button?

That way europeans like me don't need to go out of our way to investigate this :-)

Comment 17

[steve.patco]

I can adjust the firewall to add dominant countries if you tell me your CC

Comment 18

[Emilio Cobos Álvarez (:emilio)]

I'm in Spain (ES) right now

Comment 19

[Emilio Cobos Álvarez (:emilio)]

This is about whether min-height is applied on tables. Test-case incoming.

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Attachment: Test-case

Comment 21

[steve.patco]

So I noticed that the browser source code shows <!-- and --> over a good amount of code inside the Style tag. This is supposedly for the benefit of old browsers. It renders the code as comments. I am wondering if this is contributing to the problem.

Comment 22

[Boris Chiou [:boris]]

I notice this can be reproduced on mac. Change this from Windows to All

Comment 23

[Emilio Cobos Álvarez (:emilio)]

I have a patch btw, just need to take some time to fix some tests that depend on our current behavior.

Comment 24

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1651530 - Apply min/max-block-size to tables. r=mats

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Does this fix part or all of bug 307866?

Comment 26

[Mats Palmgren (inactive)]

Attachment: Testcase #2 (box-sizing: content-box)

Please add a box-sizing: content-box test as well, unless we already have that somewhere. I think this test renders correctly with your patch. (Chrome fails the min-height test AFAICT).

Comment 27

[Mats Palmgren (inactive)]

Attachment: Testcase #2 (box-sizing: content-box)

(added a couple of max-height/width tests)

Comment 28

[Emilio Cobos Álvarez (:emilio)]

(In reply to David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 from comment #25)

Does this fix part or all of bug 307866?

Only part, for the table box itself, not tbody and so on I think.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2d42454f13e1
Apply min/max-block-size to tables. r=mats

Comment 30

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/24682 for changes under testing/web-platform/tests

Comment 31

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dc056be018fe
Backed out changeset 2d42454f13e1 for mochitest failures on test_bug1642588.html. CLOSED TREE

Comment 32

[Cosmin Sabou [:CosminS]]

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=2d42454f13e124183d0444c00b5f60ca3c56b865&failure_classification_id=2&selectedTaskRun=Bi3ZjihdRyuNaMlyTgWmGg.0

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=310568760&repo=autoland
https://treeherder.mozilla.org/logviewer.html#?job_id=310565904&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/dc056be018fe

Comment 33

[Emilio Cobos Álvarez (:emilio)]

Attachment: Table contenteditable test.

So the android test is an unexpected pass. But the other one is an osx-only failure in layout/generic/test/test_bug1642588.html. The bug seems pre-existing, here's the reasoning, though I cannot reproduce the failure neither on Linux nor OSX with the patch applied.

So that test is doing something like the attached test-case. After that, the test double-clicks, then backspaces. But it turns out that before my patch that backspace made the table zero-height, and after my patch it (correctly) doesn't. It probably ended up somehow messing with the selection. But you can see clearly how after double-clicking and back-spacing the selection gets completely messed up.

Kagami, you seem to have written that test, are you ok with commenting out the table test-case with a follow-up on file for it? Or should I file the bug about the broken selection and disable the test on OSX? I can't repro and I'm pretty sure the root cause is pre-existing...

Comment 34

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1651530 - Skip a subtest in test_bug1642588.html that my changes trip on automation. r=saschanaz

Comment 35

[steve.patco]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #33)

Created attachment 9165178 [details]
Table contenteditable test.

So the android test is an unexpected pass. But the other one is an osx-only failure in layout/generic/test/test_bug1642588.html. The bug seems pre-existing, here's the reasoning, though I cannot reproduce the failure neither on Linux nor OSX with the patch applied.

So that test is doing something like the attached test-case. After that, the test double-clicks, then backspaces. But it turns out that before my patch that backspace made the table zero-height, and after my patch it (correctly) doesn't. It probably ended up somehow messing with the selection. But you can see clearly how after double-clicking and back-spacing the selection gets completely messed up.

Kagami, you seem to have written that test, are you ok with commenting out the table test-case with a follow-up on file for it? Or should I file the bug about the broken selection and disable the test on OSX? I can't repro and I'm pretty sure the root cause is pre-existing...

My site still does not pass on my android.

Comment 36

[Emilio Cobos Álvarez (:emilio)]

With which version of Firefox have you tried? The patch hasn't landed because it failed a test, so unless you've built firefox yourself with the patch it's not really surprising.

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e366e119efa0
Skip a subtest in test_bug1642588.html that my changes trip on automation. r=saschanaz

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2d3509d767bb
Apply min/max-block-size to tables. r=mats

Comment 39

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/e366e119efa0
https://hg.mozilla.org/mozilla-central/rev/2d3509d767bb

Comment 40

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot