Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

[CEF 2171] Collapsed tree nodes have no text #9965

Closed
JeffryBooher opened this issue Nov 18, 2014 · 18 comments
Closed

[CEF 2171] Collapsed tree nodes have no text #9965

JeffryBooher opened this issue Nov 18, 2014 · 18 comments

Comments

@JeffryBooher
Copy link
Contributor

Open the brackets project and collapse all top-level nodes
all disclosure triangles.

Expanding the tree node shows it but clicking other node will hide it again. Feels random.

Also, the context-selected styles are wrong.

@JeffryBooher JeffryBooher added this to the Release 1.1 milestone Nov 18, 2014
@JeffryBooher JeffryBooher changed the title [CEF 2171] Collapsed tree nodes have no text [CEF 2171]Collapsed tree nodes have no text Nov 19, 2014
@peterflynn
Copy link
Member

Crap, this appears to be a browser repaint bug. The CSS seems fine as-is, and kicking it to update by adding and removing a color: red on any given node (i.e. a no-op style change) causes that one node to start displaying correctly.

This immediately makes this CEF update seem much more risky :-(

@peterflynn
Copy link
Member

On a hunch, I tried removing the "quiet scrollbars" stuff, but that didn't fix it.

However, if I pull #project-files-container out of #sidebar and dump it as a following sibling, the issue appears to go away. Seems worth ripping out pieces of styles & layout stuff from #sidebar to try to identify what's acting as a trigger.

@peterflynn
Copy link
Member

Ugh, not looking good as far as an easy smoking gun. I reduced it a bit to this:

    <div class="main-view">
        <div id="sidebar" class="sidebar panel">
            <div class="working-set-splitview-btn btn-alt-quiet"></div>
            <div class="working-set-option-btn btn-alt-quiet"></div>

            <div id="working-set-list-container">

            </div>
            <div id="project-files-header">
                <span id="project-title" class="title"></span>
            </div>
        </div>
        <div class="fake-sidebar">
            <div id="project-files-container">
                <!-- This will contain a dynamically generated <ul> hierarchy at runtime -->
            </div>
        </div>

and

.fake-sidebar {
        width: 200px;
}

Removing the width there will make the bug go away. Uhhh...?

Notably, this removes the flexbox layout that is on a normal .sidebar. Also, it doesn't matter what the width value is -- I can make it 1000px and the labels are still all missing.

@peterflynn
Copy link
Member

Interestingly, Chrome stable just hit 39.0.2171.65 and this new CEF is also 39.0.2171.65 -- so one thing we can do is run Brackets in the browser and see if the same bug repros there. I'll start dusting off my in-browser branch so we have it available for that...

@dangoor
Copy link
Contributor

dangoor commented Nov 20, 2014

Scary. If this does repro in Chrome stable, then we'd be pretty sure to get a fix.

@peterflynn peterflynn changed the title [CEF 2171]Collapsed tree nodes have no text [CEF 2171] Collapsed tree nodes have no text Nov 20, 2014
@JeffryBooher
Copy link
Contributor Author

There's no screen grab, unfortunately, but #8133 is what we were seeing with 1916

@peterflynn
Copy link
Member

Sadly, I cannot get this to repro in browser yet. If anyone else would like to give it a try, I've pushed up pflynn/in-browser-dummy-subfolders -- a sub-branch of the in-browser version that has a more complex file tree, and a slight async delay on IO operations to approximate the timing seen in real Brackets.

@peterflynn
Copy link
Member

Aha! I can repro this with the simple in-browser version in the CEF 2171 brackets-shell. In theory that code couldn't rely on any Brackets-only shell APIs, so it should work as a repo case in vanilla cefclient too (but still not Chrome itself) -- perfect testcase for Marshall.

@JeffryBooher How hard is it to make a vanilla cefclient build? Is that in our grunt script or anything? If not, would you mind posting a copy somewhere so I could play with it?

@JeffryBooher
Copy link
Contributor Author

@peterflynn you will find it in your deps/cef folder under brackets-shell. Open cefclient.vcxproj in Visual Studio and build/run it. That will get you a basic web browser with an address bar that you should be able to enter a file:// URL to your test page

@peterflynn
Copy link
Member

@JeffryBooher I get a linker error: "cannot open input file '...\deps\cef\out\Debug\lib\libcef_dll_wrapper.lib'" (not sure why it's looking there since I had selected the Release configuration, but \lib is empty in both Debug & Release anyway...)

@peterflynn
Copy link
Member

I found a libcef_dll_wrapper.sln in the same folder, but if I open that and make a Debug build (still no idea why it's not looking in the Release folder), then I get more link errors in the cefclient project. Most of them are "mismatch detected for '_ITERATOR_DEBUG_LEVEL': value '2' doesn't match value '0' in cefclient.obj." Any ideas?

@peterflynn
Copy link
Member

Aha, looks like you just can't make Release builds of cefclient. I got a Debug build working though -- and hurrah!, the labels are consistently broken there too.

Hopefully this is good enough to package into a testcase now, since there's a clear rendering diff between CEF and Chromium.

@JeffryBooher
Copy link
Contributor Author

@peterflynn my bad, you were supposed to open cefclient2010.sln in Visual Studio which will build all of the dependencies.

@peterflynn peterflynn self-assigned this Nov 21, 2014
@RaymondLim
Copy link
Contributor

I found the culprit. If I comment out the following CSS properties that can trigger the GPU mode, then the issue is fixed.

/** Overriding jsTreeTheme.less
*/
.jstree-brackets .jstree-no-dots .jstree-open > ins {
    background-position: 7px -8px;
/*    -webkit-transform: translateZ(0) rotate(90deg);
    -webkit-transition: -webkit-transform 190ms cubic-bezier(.01, .91, 0, .99);
    -webkit-filter: drop-shadow(1px 0 1px @bc-shadow);*/

/*    .dark & {
        -webkit-filter: drop-shadow(1px 0 1px @dark-bc-shadow);
    }*/
}

.jstree-brackets .jstree-no-dots .jstree-closed > ins {
    background-position: 7px -8px;
/*    -webkit-transform: translateZ(0);  Need this to make sure that the svg isn't blurry on retina. 
    -webkit-transition: -webkit-transform 90ms cubic-bezier(.01, .91, 0, .99);
    -webkit-filter: drop-shadow(1px 0 1px @bc-shadow);*/

/*    .dark & {
        -webkit-filter: drop-shadow(1px 0 1px @dark-bc-shadow);
    }*/
}

So this is definitely related to #9978 which is caused by translateZ, but we also need to report that -webkit-filter property can trigger the GPU mode and causes text not to render.

@RaymondLim
Copy link
Contributor

Actually, the real culprit is not -webkit-filter as mentioned in my previous comment. I still see the very first item (either file/folder) of the tree not rendered with the above commented code. If I instead comment out the following lines in jsTreeTheme.less file, then the entire tree is rendered.

.jstree-brackets li > a {
/*    @jstree-icon-text-overlap: 2px;
    padding-left: (@jstree-sprite-size - @jstree-icon-backindent - @jstree-icon-text-overlap + 10000px);
    margin-left: -10000px;*/
    display: block;
}

It seems like our trick to render the tree offscreen (in negative coordinate) and then bring it back with margin-left and padding-left properties are ignored by latest Chromium.

@peterflynn
Copy link
Member

In my in-browser/cefclient testcase, removing -webkit-filter fixes the persistent bug, but the labels still briefly disappear whenever the tree is updated. Removing the huge padding-left and margin-left from the rule above fully fixes the bug.

I searched for other places where we use large values in CSS, and luckily nothing else looks too similar to this. (.dummy-text uses a very large negative top, but it's meant to stay offscreen -- not be pushed back onscreen by a similarly large positive value).

@peterflynn
Copy link
Member

I filed https://code.google.com/p/chromiumembedded/issues/detail?id=1452 on the CEF bug, but it does appear that we can safely work around it in this case by removing the margin/padding hack, which is no longer needed with our new React-based tree widget.

@peterflynn
Copy link
Member

Confirmed fixed in Brackets. Although the underlying CEF/Chromium issue is still present (see bug linked above), I don't think this needs to stay open tracking a fix since we don't have reason to believe anything else in Brackets will trigger that bug. Closing.

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

No branches or pull requests

4 participants